Skip to content

Commit

Permalink
Fix up timeouts in profiling GetStatusAction (#108844)
Browse files Browse the repository at this point in the history
We shouldn't be using `ackTimeout()` for something unrelated to cluster
state acks, and we should be passing in the master-node timeout at
construction time. This commit fixes these things.
  • Loading branch information
DaveCTurner committed May 21, 2024
1 parent d170383 commit 5ae0566
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ public void setupCluster() {

public void testTimeoutIfResourcesNotCreated() throws Exception {
updateProfilingTemplatesEnabled(false);
GetStatusAction.Request request = new GetStatusAction.Request();
request.waitForResourcesCreated(true);
// shorter than the default timeout to avoid excessively long execution
request.ackTimeout(TimeValue.timeValueSeconds(15));
GetStatusAction.Request request = new GetStatusAction.Request(
TimeValue.THIRTY_SECONDS,
true,
// shorter than the default timeout to avoid excessively long execution:
TimeValue.timeValueSeconds(15)
);

GetStatusAction.Response response = client().execute(GetStatusAction.INSTANCE, request).get();
assertEquals(RestStatus.REQUEST_TIMEOUT, response.status());
Expand All @@ -43,8 +45,7 @@ public void testTimeoutIfResourcesNotCreated() throws Exception {

public void testNoTimeoutIfNotWaiting() throws Exception {
updateProfilingTemplatesEnabled(false);
GetStatusAction.Request request = new GetStatusAction.Request();
request.waitForResourcesCreated(false);
GetStatusAction.Request request = new GetStatusAction.Request(TimeValue.THIRTY_SECONDS, false, randomTimeValue());

GetStatusAction.Response response = client().execute(GetStatusAction.INSTANCE, request).get();
assertEquals(RestStatus.OK, response.status());
Expand All @@ -54,10 +55,12 @@ public void testNoTimeoutIfNotWaiting() throws Exception {

public void testWaitsUntilResourcesAreCreated() throws Exception {
updateProfilingTemplatesEnabled(true);
GetStatusAction.Request request = new GetStatusAction.Request();
// higher timeout since we have more shards than usual
request.ackTimeout(TimeValue.timeValueSeconds(120));
request.waitForResourcesCreated(true);
GetStatusAction.Request request = new GetStatusAction.Request(
TimeValue.THIRTY_SECONDS,
true,
// higher timeout since we have more shards than usual:
TimeValue.timeValueSeconds(120)
);

GetStatusAction.Response response = client().execute(GetStatusAction.INSTANCE, request).get();
assertEquals(RestStatus.OK, response.status());
Expand All @@ -67,9 +70,7 @@ public void testWaitsUntilResourcesAreCreated() throws Exception {

public void testHasData() throws Exception {
doSetupData();
GetStatusAction.Request request = new GetStatusAction.Request();
request.waitForResourcesCreated(true);

GetStatusAction.Request request = new GetStatusAction.Request(TimeValue.THIRTY_SECONDS, true, TimeValue.THIRTY_SECONDS);
GetStatusAction.Response response = client().execute(GetStatusAction.INSTANCE, request).get();
assertEquals(RestStatus.OK, response.status());
assertTrue(response.isResourcesCreated());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@

package org.elasticsearch.xpack.profiling.action;

import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.action.ActionType;
import org.elasticsearch.action.support.master.AcknowledgedRequest;
import org.elasticsearch.action.support.master.MasterNodeRequest;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
Expand Down Expand Up @@ -125,29 +127,39 @@ public RestStatus status() {
}
}

public static class Request extends AcknowledgedRequest<GetStatusAction.Request> {
private boolean waitForResourcesCreated;
public static class Request extends MasterNodeRequest<Request> {
private final boolean waitForResourcesCreated;
private final TimeValue waitForResourcesCreatedTimeout;

public Request(StreamInput in) throws IOException {
super(in);
waitForResourcesCreatedTimeout = in.readTimeValue();
waitForResourcesCreated = in.readBoolean();
}

public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
public Request(TimeValue masterNodeTimeout, boolean waitForResourcesCreated, TimeValue waitForResourcesCreatedTimeout) {
super(masterNodeTimeout);
this.waitForResourcesCreated = waitForResourcesCreated;
this.waitForResourcesCreatedTimeout = waitForResourcesCreatedTimeout;
}

public boolean waitForResourcesCreated() {
return waitForResourcesCreated;
}

public void waitForResourcesCreated(boolean waitForResourcesCreated) {
this.waitForResourcesCreated = waitForResourcesCreated;
public TimeValue waitForResourcesCreatedTimeout() {
return waitForResourcesCreatedTimeout;
}

@Override
public ActionRequestValidationException validate() {
return null;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeTimeValue(waitForResourcesCreatedTimeout);
out.writeBoolean(waitForResourcesCreated);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ protected void masterOperation(
ActionListener<GetStatusAction.Response> listener
) {
if (request.waitForResourcesCreated()) {
createAndRegisterListener(listener, request.ackTimeout());
createAndRegisterListener(listener, request.waitForResourcesCreatedTimeout());
} else {
resolver.execute(state, listener);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package org.elasticsearch.xpack.profiling.rest;

import org.elasticsearch.client.internal.node.NodeClient;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.Scope;
Expand All @@ -18,7 +19,6 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestUtils.getAckTimeout;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.PUBLIC)
Expand All @@ -36,10 +36,11 @@ public String getName() {

@Override
protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) {
GetStatusAction.Request request = new GetStatusAction.Request();
request.ackTimeout(getAckTimeout(restRequest));
request.masterNodeTimeout(getMasterNodeTimeout(restRequest));
request.waitForResourcesCreated(restRequest.paramAsBoolean("wait_for_resources_created", false));
final var request = new GetStatusAction.Request(
getMasterNodeTimeout(restRequest),
restRequest.paramAsBoolean("wait_for_resources_created", false),
restRequest.paramAsTime("timeout", TimeValue.THIRTY_SECONDS)
);
return channel -> client.execute(
GetStatusAction.INSTANCE,
request,
Expand Down

0 comments on commit 5ae0566

Please sign in to comment.