Skip to content

Conversation

@Sanne
Copy link
Contributor

@Sanne Sanne commented Jan 26, 2018

…s successful

Closes #24069

Please consider backporting, I'm seeing quite some users affected by this.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@mayya-sharipova
Copy link
Contributor

@elasticmachine ok to test

@Sanne
Copy link
Contributor Author

Sanne commented Jan 31, 2018

Hi @javanna looks like I passed the paperwork barriers ^ ;) In your hands now?

@javanna
Copy link
Member

javanna commented Jan 31, 2018

yea @Sanne I'll try to figure out how to test this in the coming days.

@Sanne
Copy link
Contributor Author

Sanne commented Jan 31, 2018

yes that's the hard part. I actually feel bad about sending a PR without tests :)

Personally I would use Byteman for such cases, it helps re-create a scenario with parallel threads as you can block them on specific points and control the flow (Byteman was originally created to address the testing needs of our JTA library Narayana, later we started using it on other projects too).
I didn't feel like introducing a Byteman based test as you'd need the new dependency and an agent running: might be a bit intrusive on the build, especially as I wouldn't know how you want things organized.

@nik9000
Copy link
Member

nik9000 commented Mar 13, 2018

I was working to reproduce another issue but I think I stumbled into a fairly good test for this one. See this gist.

@nik9000
Copy link
Member

nik9000 commented Mar 13, 2018

I'd certainly prefer to have a more targeted test, but without one I think this is fairly useful.

@nik9000 nik9000 added >bug :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch labels Mar 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member

javanna commented Mar 15, 2018

hi @Sanne I think it was a mistake on my end to set this timeout, I also misunderstood what it was meant for. I would then prefer to leave the apache http async default if it's already ok, but it seems like its default value is -1 (system default) and not 0. Could you comment on this?

@javanna
Copy link
Member

javanna commented Mar 15, 2018

One more thing, I am ok with the test that @nik9000 proposed above, maybe you can update the PR so we can finally get it in (sorry it took ages, I feel bad about that).

@nik9000
Copy link
Member

nik9000 commented Mar 27, 2018

@Sanne, would you like to pick up this PR and add a test, maybe similar to the one I proposed? Would you be OK if I picked it up? I think it is worth fixing this issue and you found the issue so I figure it is only right you get your name in the git history.

@Sanne
Copy link
Contributor Author

Sanne commented Apr 18, 2018

hi @nik9000 ! I appreciate the kind offer but feel free to take ownership of it.

@javanna : I no longer have the environment which I used to reliably reproduce this so I wouldn't be able to test that. I'm sure my patch resolved the problem, but maybe -1 would work as well.

@javanna
Copy link
Member

javanna commented Apr 19, 2018

No worries, we have taken ownership of this. I started working on some changes around timeouts including this one and adding tests, but I need first #29211 to go in otherwise resolving merge conflicts becomes a nightmare.

@javanna
Copy link
Member

javanna commented May 4, 2018

thanks once again @Sanne for opening this and for the detailed analysis. Sorry that it took so long to integrate the suggested changes. I finally opened a PR with a slightly different fix and with tests that fail without the fix. I am closing this PR as it's superseded by #30384 .

@javanna javanna closed this May 4, 2018
@Sanne
Copy link
Contributor Author

Sanne commented May 4, 2018

awesome, thanks! Looking forward to see this benefit everyone else.

@Sanne Sanne deleted the HttpClientTimeouts branch May 4, 2018 10:47
@rafaelbattesti
Copy link

rafaelbattesti commented May 9, 2018

Hi @javanna . What would you recommend as I have a requirement for 5.6.9? I tried setConnectionRequestTimeout(0) and setConnectionRequestTimeout(-1) to no avail.
I am running my application in a docker container on a k8 cluster. Requests are successful when sent from my local environment, but I get the timeout from org.apache.http.nio.pool.RouteSpecificPool.timeout(RouteSpecificPool.java:168) when running on the cluster.
Thanks!

@javanna
Copy link
Member

javanna commented May 9, 2018

hi @rafaelbattesti I'd assume it's a different problem if explicitly setting the connection request timeout doesn't help. Could you share your initialization code for the client, and the full stacktrace of the exception please?

@rafaelbattesti
Copy link

@javanna I found this has to do with the network setup.

@javanna
Copy link
Member

javanna commented May 9, 2018

@rafaelbattesti glad to know you found the cause, would you mind sharing what caused the problem?

@rafaelbattesti
Copy link

@javanna I wouldn't mind if I had reached a precise diagnostic. I can tell it's a network setup because a basic curl from my container timed out, while it gets me the expected response from my local machine. I am contacting the K8 admin team for troubleshooting. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants