Test: add setting to change request timeout for rest client #25201
Conversation
This commit adds a setting to change the request timeout for the rest client. This is useful as the default timeout is 30s, which is also the same default for calls like cluster health. If both are the same then the response from the cluster health api will not be received as the client usually times out first making test failures harder to debug. Relates elastic#25185
This commit adds a setting to change the request timeout for the rest client. This is useful as the default timeout is 30s, which is also the same default for calls like cluster health. If both are the same then the response from the cluster health api will not be received as the client usually times out first making test failures harder to debug. Relates #25185
This commit adds a setting to change the request timeout for the rest client. This is useful as the default timeout is 30s, which is also the same default for calls like cluster health. If both are the same then the response from the cluster health api will not be received as the client usually times out first making test failures harder to debug. Relates #25185
|
Thanks @nik9000 for taking a look |
| final String requestTimeoutString = settings.get(CLIENT_RETRY_TIMEOUT); | ||
| if (requestTimeoutString != null) { | ||
| final TimeValue maxRetryTimeout = TimeValue.parseTimeValue(requestTimeoutString, CLIENT_RETRY_TIMEOUT); | ||
| builder.setMaxRetryTimeoutMillis(Math.toIntExact(maxRetryTimeout.getMillis())); |
There was a problem hiding this comment.
The title of the PR mentions changing the request timeout: we have connect timeout, socket timeout and our own max retry timeout to make sure that retries that don't go over a certain timeout. Did you mean to also change the socket timeout here? That may make sense... Also, do you think it is still a good default to have both max retry and socket timeout set to the same value?
There was a problem hiding this comment.
our own max retry timeout to make sure that retries that don't go over a certain timeout
Something separate but I wonder if there should be a difference between retry timeout and actual response timeout? Or maybe retry timeout should always be >= socket timeout?
Did you mean to also change the socket timeout here?
I should have. I'll open a new PR shortly for that. Thanks for catching it.
There was a problem hiding this comment.
The difference between socket timeout and max retry is that socket timeout is something set to the http client directly, for each single request. A socket timeout is the timeout when waiting for individual packets, it triggers a timeout whenever no packets come back at all for that many seconds. Max retry is something that we introduced in our own RestClient given that we may retry a request against multiple hosts, to give the guarantee that a request overall (including retries) won't take longer than the set max retry timeout. It can happen that even with a single retry, the socket timeout doesn't trip as something is slowly coming back, but the max retry trips as the request overall is taking too long. Maybe I should have named it differently. And I am not sure whether the same default value is good for both. I am also not sure if we should add validation that max retry is greater or equal than socket timeout.
In elastic#25201, a setting was added to allow setting the retry timeout for the rest client under the impression that this would allow requests to go longer than 30s. However, there is also a socket timeout that needs to be set to greater than 30s, which this change adds a setting for.
In #25201, a setting was added to allow setting the retry timeout for the rest client under the impression that this would allow requests to go longer than 30s. However, there is also a socket timeout that needs to be set to greater than 30s, which this change adds a setting for.
In #25201, a setting was added to allow setting the retry timeout for the rest client under the impression that this would allow requests to go longer than 30s. However, there is also a socket timeout that needs to be set to greater than 30s, which this change adds a setting for.
In #25201, a setting was added to allow setting the retry timeout for the rest client under the impression that this would allow requests to go longer than 30s. However, there is also a socket timeout that needs to be set to greater than 30s, which this change adds a setting for.
* master: (27 commits) Refactor TransportShardBulkAction.executeUpdateRequest and add tests Make sure range queries are correctly profiled. (elastic#25108) Test: allow setting socket timeout for rest client (elastic#25221) Migration docs for elastic#25080 (elastic#25218) Remove `discovery.type` BWC layer from the EC2/Azure/GCE plugins elastic#25080 When stopping via systemd only kill the JVM, not its control group (elastic#25195) Remove PrefixAnalyzer, because it is no longer used. Internal: Remove Strings.cleanPath (elastic#25209) Docs: Add note about which secure settings are valid (elastic#25212) Indices.rollover/10_basic should refresh to make the doc visible in lucene stats Port support for commercial GeoIP2 databases from Logstash. (elastic#24889) [DOCS] Add ML node to node.asciidoc (elastic#24495) expose simple pattern tokenizers (elastic#25159) Test: add setting to change request timeout for rest client (elastic#25201) Fix secure repository-hdfs tests on JDK 9 Add target_field parameter to gsub, join, lowercase, sort, split, trim, uppercase (elastic#24133) Add Cross Cluster Search support for scroll searches (elastic#25094) Adapt skip version in rest-api-spec/test/indices.rollover/20_max_doc_condition.yml Rollover max docs should only count primaries (elastic#24977) Add remote cluster infrastructure to fetch discovery nodes. (elastic#25123) ...
4 participants
This commit adds a setting to change the request timeout for the rest client. This is useful as the
default timeout is 30s, which is also the same default for calls like cluster health. If both are
the same then the response from the cluster health api will not be received as the client usually
times out first making test failures harder to debug.
Relates #25185