Skip to content

Increase timeouts for Rest test client ...#19042

Merged
ywelsch merged 1 commit intoelastic:masterfrom
ywelsch:fix/rest-test-client-timeouts
Jun 23, 2016
Merged

Increase timeouts for Rest test client ...#19042
ywelsch merged 1 commit intoelastic:masterfrom
ywelsch:fix/rest-test-client-timeouts

Conversation

@ywelsch
Copy link
Copy Markdown
Contributor

@ywelsch ywelsch commented Jun 23, 2016

... to fix failing tests.

@ywelsch ywelsch added >test Issues or PRs that are addressing/adding tests :Core/Infra/REST API REST infrastructure and utilities labels Jun 23, 2016
Copy link
Copy Markdown
Contributor

@javanna javanna Jun 23, 2016

Choose a reason for hiding this comment

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

do we want to instead make socket timeout an argument to createDefaultHttpClient given most of the users may want to do this but leaving the rest as-is, as we are doing here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've also increased the connect and connectionrequest timeouts. I'm not sure if users may only want to increase socket timeout...

Copy link
Copy Markdown
Contributor

@javanna javanna Jun 23, 2016

Choose a reason for hiding this comment

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

ok I missed that. I wouldn't increase just everything here. Connection request timeout is about retrieving tcp connections from the internal pool that the http client has, I wouldn't touch that.
As for for connect timeout, not sure either, especially because we are not hitting a connection timeout but a socket timeout in our tests.

I don't like the fact that we have to copy the whole createDefaultHttpClient method here. But this is a usability concern that we can address in a separate issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agree, we should make it easier to adapt this one setting.

I've just modified the socket timeout as per your suggestion.

@ywelsch ywelsch force-pushed the fix/rest-test-client-timeouts branch from 94e427e to 0a5ff45 Compare June 23, 2016 12:03
@ywelsch ywelsch merged commit a5908a5 into elastic:master Jun 23, 2016
jasontedor added a commit that referenced this pull request Jun 23, 2016
* master: (416 commits)
  docs: removed obsolete information, percolator queries are not longer loaded into jvm heap memory.
  Upgrade JNA to 4.2.2 and remove optionality
  [TEST] Increase timeouts for Rest test client (#19042)
  Update migrate_5_0.asciidoc
  Add ThreadLeakLingering option to Rest client tests
  Add a MultiTermAwareComponent marker interface to analysis factories. #19028
  Attempt at fixing IndexStatsIT.testFilterCacheStats.
  Fix docs build.
  Move templates out of the Search API, into lang-mustache module
  revert - Inline reroute with process of node join/master election (#18938)
  Build valid slices in SearchSourceBuilderTests
  Docs: Convert aggs/misc to CONSOLE
  Docs: migration notes for _timestamp and _ttl
  Group client projects under :client
  [TEST] Add client-test module and make client tests use randomized runner directly
  Move upgrade test to upgrade from version 2.3.3
  Tasks: Add completed to the mapping
  Fail to start if plugin tries broken onModule
  Remove duplicated read byte array methods
  Rename `fields` to `stored_fields` and add `docvalue_fields`
  ...
@clintongormley clintongormley added :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch and removed :Core/Infra/REST API REST infrastructure and utilities labels Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >test Issues or PRs that are addressing/adding tests v5.0.0-alpha4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants