Skip to content

Commit

Permalink
Fix timeouts which override master-node timeouts
Browse files Browse the repository at this point in the history
There's a couple of APIs whose `?timeout` parameter also sets the
`?master_timeout` timeout unless the latter is specified separately.
Today this logic happens within the relevant requests' `timeout()`
setters, but it'd be enormously preferable to implement this
REST-layer-specific logic in the REST layer instead, and configure the
timeouts in the transport-layer requests explicitly. This commit does
so.

Relates elastic#107984
  • Loading branch information
DaveCTurner committed Apr 29, 2024
1 parent 30d31bf commit 6655d9b
Show file tree
Hide file tree
Showing 10 changed files with 25 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ public void testNodeRemovalFromRedClusterWithTimeout() throws Exception {
PrevalidateNodeRemovalRequest req = PrevalidateNodeRemovalRequest.builder()
.setNames(node2)
.build()
.masterNodeTimeout(TimeValue.timeValueSeconds(1))
.timeout(TimeValue.timeValueSeconds(1));
PrevalidateNodeRemovalResponse resp = client().execute(PrevalidateNodeRemovalAction.INSTANCE, req).get();
assertFalse("prevalidation result should return false", resp.getPrevalidation().isSafe());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ public void testCorruptFileAndRecover() throws InterruptedException, IOException
ClusterHealthResponse health = clusterAdmin().health(
new ClusterHealthRequest("test").waitForGreenStatus()
// sometimes due to cluster rebalancing and random settings default timeout is just not enough.
.masterNodeTimeout(TimeValue.timeValueMinutes(5))
.timeout(TimeValue.timeValueMinutes(5))
.waitForNoRelocatingShards(true)
).actionGet();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ public void testRandomDirectoryIOExceptions() throws IOException, InterruptedExc
}
ClusterHealthResponse clusterHealthResponse = clusterAdmin()
// it's OK to timeout here
.health(new ClusterHealthRequest(new String[] {}).waitForYellowStatus().timeout(TimeValue.timeValueSeconds(5)))
.health(
new ClusterHealthRequest(new String[] {}).waitForYellowStatus()
.masterNodeTimeout(TimeValue.timeValueSeconds(5))
.timeout(TimeValue.timeValueSeconds(5))
)
.get();
final int numDocs;
final boolean expectAllShardsFailed;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,6 @@ public TimeValue timeout() {

public ClusterHealthRequest timeout(TimeValue timeout) {
this.timeout = timeout;
if (masterNodeTimeout == DEFAULT_MASTER_NODE_TIMEOUT) {
masterNodeTimeout = timeout;
}
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,6 @@ public TimeValue timeout() {

public PrevalidateNodeRemovalRequest timeout(TimeValue timeout) {
this.timeout = timeout;
if (masterNodeTimeout == DEFAULT_MASTER_NODE_TIMEOUT) {
masterNodeTimeout = timeout;
}
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,12 @@ public static ClusterHealthRequest fromRequest(final RestRequest request) {
final ClusterHealthRequest clusterHealthRequest = new ClusterHealthRequest(indices);
clusterHealthRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterHealthRequest.indicesOptions()));
clusterHealthRequest.local(request.paramAsBoolean("local", clusterHealthRequest.local()));
clusterHealthRequest.masterNodeTimeout(getMasterNodeTimeout(request));
clusterHealthRequest.timeout(request.paramAsTime("timeout", clusterHealthRequest.timeout()));
if (request.hasParam("master_timeout")) {
clusterHealthRequest.masterNodeTimeout(getMasterNodeTimeout(request));
} else {
clusterHealthRequest.masterNodeTimeout(clusterHealthRequest.timeout());
}
String waitForStatus = request.param("wait_for_status");
if (waitForStatus != null) {
clusterHealthRequest.waitForStatus(ClusterHealthStatus.valueOf(waitForStatus.toUpperCase(Locale.ROOT)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,12 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
.setIds(ids)
.setExternalIds(externalIds)
.build();
prevalidationRequest.masterNodeTimeout(getMasterNodeTimeout(request));
prevalidationRequest.timeout(request.paramAsTime("timeout", prevalidationRequest.timeout()));
if (request.hasParam("master_timeout")) {
prevalidationRequest.masterNodeTimeout(getMasterNodeTimeout(request));
} else {
prevalidationRequest.masterNodeTimeout(prevalidationRequest.timeout());
}
return channel -> client.execute(
PrevalidateNodeRemovalAction.INSTANCE,
prevalidationRequest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,9 @@ private ClusterHealthStatus ensureColor(
String color = clusterHealthStatus.name().toLowerCase(Locale.ROOT);
String method = "ensure" + Strings.capitalize(color);

ClusterHealthRequest healthRequest = new ClusterHealthRequest(indices).timeout(timeout)
ClusterHealthRequest healthRequest = new ClusterHealthRequest(indices)
.masterNodeTimeout(timeout)
.timeout(timeout)
.waitForStatus(clusterHealthStatus)
.waitForEvents(Priority.LANGUID)
.waitForNoRelocatingShards(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,8 @@ public ClusterHealthStatus ensureGreen(String... indices) {
*/
public ClusterHealthStatus ensureGreen(TimeValue timeout, String... indices) {
ClusterHealthResponse actionGet = clusterAdmin().health(
new ClusterHealthRequest(indices).timeout(timeout)
new ClusterHealthRequest(indices).masterNodeTimeout(timeout)
.timeout(timeout)
.waitForGreenStatus()
.waitForEvents(Priority.LANGUID)
.waitForNoRelocatingShards(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,9 @@ private ClusterHealthStatus ensureColor(
String color = clusterHealthStatus.name().toLowerCase(Locale.ROOT);
String method = "ensure" + Strings.capitalize(color);

ClusterHealthRequest healthRequest = new ClusterHealthRequest(indices).timeout(timeout)
ClusterHealthRequest healthRequest = new ClusterHealthRequest(indices)
.masterNodeTimeout(timeout)
.timeout(timeout)
.waitForStatus(clusterHealthStatus)
.waitForEvents(Priority.LANGUID)
.waitForNoRelocatingShards(true)
Expand Down

0 comments on commit 6655d9b

Please sign in to comment.