Skip to content

Test: add setting to change request timeout for rest client #25201

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 13, 2017
Merged

Test: add setting to change request timeout for rest client #25201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...upgrade/src/test/java/org/elasticsearch/upgrades/UpgradeClusterClientYamlTestSuiteIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import com.carrotsearch.randomizedtesting.annotations.TimeoutSuite;
import org.apache.lucene.util.TimeUnits;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.rest.ESRestTestCase;
import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate;
import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase;

Expand All @@ -46,5 +48,14 @@ public UpgradeClusterClientYamlTestSuiteIT(ClientYamlTestCandidate testCandidate
public static Iterable<Object[]> parameters() throws Exception {
return createParameters();
}

@Override
protected Settings restClientSettings() {
return Settings.builder().put(super.restClientSettings())
// increase the timeout so that we can actually see the result of failed cluster health
// calls that have a default timeout of 30s
.put(ESRestTestCase.CLIENT_RETRY_TIMEOUT, "40s")
.build();
}
}

8 changes: 8 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.client.RestClientBuilder;
import org.elasticsearch.common.io.PathUtils;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
Expand Down Expand Up @@ -67,6 +68,7 @@
public abstract class ESRestTestCase extends ESTestCase {
public static final String TRUSTSTORE_PATH = "truststore.path";
public static final String TRUSTSTORE_PASSWORD = "truststore.password";
public static final String CLIENT_RETRY_TIMEOUT = "client.retry.timeout";

/**
* Convert the entity from a {@link Response} into a map of maps.
Expand Down Expand Up @@ -338,6 +340,12 @@ protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOE
}
builder.setDefaultHeaders(defaultHeaders);
}

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()));
Copy link
Member

@javanna javanna Jun 14, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@jaymode jaymode Jun 14, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@javanna javanna Jun 14, 2017

Choose a reason for hiding this comment

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

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.

}
return builder.build();
}

Expand Down