Skip to content

Commit

Permalink
Fix timeouts which override master-node timeouts (#107992)
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 #107984
  • Loading branch information
DaveCTurner committed Apr 29, 2024
1 parent 68b5336 commit 1f1ab06
Show file tree
Hide file tree
Showing 10 changed files with 23 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,8 @@ 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,8 @@ 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 1f1ab06

Please sign in to comment.