Jetty http client async support#341
Conversation
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
meltsufin
left a comment
meltsufin
left a comment
There was a problem hiding this comment.
@olamy @jmcc0nn3ll I built the image and tested it against a sample app. I had to make a small change to the pom to make it work, but it seems functional. See some of my comments around the timeouts though.
| private static HttpClient createHttpClient() { | ||
| try { | ||
| HttpClientTransportOverHTTP transport = new HttpClientTransportOverHTTP(); | ||
| // FIXME what sort of customize SslContextFactory we want to use here?? |
There was a problem hiding this comment.
It doesn't look like any requests go over SSL. So, we should be ok with not using one at all.
There was a problem hiding this comment.
correct we only have to remove the FIXME :-)
| SslContextFactory sslContextFactory = new SslContextFactory(); | ||
| HttpClient httpClient = new HttpClient( transport, sslContextFactory ); | ||
| httpClient.setMaxConnectionsPerDestination( VmApiProxyEnvironment.MAX_CONCURRENT_API_CALLS ); | ||
| httpClient.setConnectTimeout( ADDITIONAL_HTTP_TIMEOUT_BUFFER_MS ); |
There was a problem hiding this comment.
Originally it was timeoutMs + ADDITIONAL_HTTP_TIMEOUT_BUFFER_MS. I know that timeoutMs is not available at this point, but maybe there is a workaround?
There was a problem hiding this comment.
@meltsufin AFAICS the connection timeout was alway set to DEFAULT_RPC_TIMEOUT_MS. So I will add to have similar behaviour.
| params.setBooleanParameter(CoreConnectionPNames.STALE_CONNECTION_CHECK, Boolean.FALSE); | ||
| request.setParams(params); | ||
|
|
||
| request.timeout( ADDITIONAL_HTTP_TIMEOUT_BUFFER_MS, TimeUnit.MILLISECONDS ); |
There was a problem hiding this comment.
yup definitely missing! I will fix that.
Signed-off-by: olivier lamy <olamy@apache.org>
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
|
LGTM |
3 participants
Re-creating the PR from a local branch.
I added a small fix to the pom to make the image include jetty-client.jar.
@olamy