Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure Guzzle clients #64

Merged
merged 3 commits into from
Jan 30, 2018
Merged

Configure Guzzle clients #64

merged 3 commits into from
Jan 30, 2018

Conversation

thewilkybarkid
Copy link
Member

No description provided.

'handler' => $this->app['hypothesis.guzzle.handler'],
'timeout' => 0.9,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want fast failures as Journal has a 1-second timeout. This has to be less than that otherwise Annotations won't see an error (Journal will just have disconnected).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's reasonable to think that the rest of the code being executed by this service will have a 100ms overhead, anyway

@@ -224,7 +224,10 @@ public function __construct(string $environment = 'dev')

return new Client([
'base_uri' => $this->app['hypothesis']['api_url'],
'connect_timeout' => 0.5,
'decode_content' => 'gzip',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth doing as this is external.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirmed that the current hypothesis-dummy supports this for testing (happens at the cURL level, which the local tests don't hit), though elifesciences/hypothesis-dummy#3 will turn it off (removes Nginx; could add it to the PHP code?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should degrade automatically if the response is not gzipped as there's no Content-Encoding header? Which would be a difference between end2end and prod but not critical?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, cURL handles it all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands, this should not explode with any version of hypothesis-dummy then

@thewilkybarkid
Copy link
Member Author

We might want to use HTTP/2 for Hypothesis (their API supports it), but not sure what to make of guzzle/guzzle#1249 (means upgrading cURL anyhow).

@thewilkybarkid thewilkybarkid changed the title [WIP] Configure Guzzle clients [WIP] Jan 29, 2018
@thewilkybarkid thewilkybarkid changed the title [WIP] Configure Guzzle clients Jan 29, 2018
@thewilkybarkid
Copy link
Member Author

thewilkybarkid commented Jan 30, 2018

Ignoring HTTP/2 for now, will need some experimentation. (Opened ELPP-3406.)

@thewilkybarkid thewilkybarkid merged commit 0cd665f into develop Jan 30, 2018
@thewilkybarkid thewilkybarkid deleted the guzzle-configuration branch January 30, 2018 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants