Skip to content

Commit

Permalink
Make master-node timeout less implicit (#108414)
Browse files Browse the repository at this point in the history
Removes the default constructors for `MasterRequest`,
`MasterReadRequest` and `AcknowledgedRequest` in favour of constructors
which require subclasses to specify the relevant timeouts. This will
avoid bugs like #107857 which are caused by a missing `super()` call.

Also deprecates and renames the default to make it clear it should not
be used in new code.

Relates #107984
  • Loading branch information
DaveCTurner committed May 13, 2024
1 parent e352345 commit 0b2e558
Show file tree
Hide file tree
Showing 214 changed files with 549 additions and 165 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeBoolean(dryRun);
}

public Request() {}
public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public boolean dryRun() {
return dryRun;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public void writeTo(StreamOutput out) throws IOException {
}

public Request(String[] names) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
this.names = names;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ private GetDataStreamGlobalRetentionAction() {/* no instances */}

public static final class Request extends MasterNodeReadRequest<Request> {

public Request() {}
public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public Request(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ public Request(StreamInput in) throws IOException {
super(in);
}

public Request() {}
public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

@Override
public ActionRequestValidationException validate() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public void writeTo(StreamOutput out) throws IOException {
}

public Request(@Nullable TimeValue defaultRetention, @Nullable TimeValue maxRetention) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.globalRetention = new DataStreamGlobalRetention(defaultRetention, maxRetention);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public class ClusterAllocationExplainRequest extends MasterNodeRequest<ClusterAl
* Create a new allocation explain request to explain any unassigned shard in the cluster.
*/
public ClusterAllocationExplainRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.index = null;
this.shard = null;
this.primary = null;
Expand All @@ -73,6 +74,7 @@ public ClusterAllocationExplainRequest(StreamInput in) throws IOException {
* Package private for testing.
*/
ClusterAllocationExplainRequest(String index, int shard, boolean primary, @Nullable String currentNode) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.index = index;
this.shard = shard;
this.primary = primary;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
import java.io.IOException;

public class DesiredBalanceRequest extends MasterNodeReadRequest<DesiredBalanceRequest> {
public DesiredBalanceRequest() {}
public DesiredBalanceRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public DesiredBalanceRequest(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state)
public static class Request extends MasterNodeReadRequest<Request> {

public Request(TaskId parentTaskId) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
setParentTask(parentTaskId);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public AddVotingConfigExclusionsRequest(String... nodeNames) {
* @param timeout How long to wait for the added exclusions to take effect and be removed from the voting configuration.
*/
public AddVotingConfigExclusionsRequest(String[] nodeIds, String[] nodeNames, TimeValue timeout) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
if (timeout.compareTo(TimeValue.ZERO) < 0) {
throw new IllegalArgumentException("timeout [" + timeout + "] must be non-negative");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ public class ClearVotingConfigExclusionsRequest extends MasterNodeRequest<ClearV
/**
* Construct a request to remove all the voting config exclusions from the cluster state.
*/
public ClearVotingConfigExclusionsRequest() {}
public ClearVotingConfigExclusionsRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public ClearVotingConfigExclusionsRequest(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public class GetDesiredNodesAction extends ActionType<GetDesiredNodesAction.Resp
}

public static class Request extends MasterNodeReadRequest<Request> {
public Request() {}
public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public Request(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ public ClusterState afterBatchExecution(ClusterState clusterState, boolean clust
}

public static class Request extends AcknowledgedRequest<Request> {
public Request() {}
public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
}

public Request(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class UpdateDesiredNodesRequest extends AcknowledgedRequest<UpdateDesired
}

public UpdateDesiredNodesRequest(String historyID, long version, List<DesiredNode> nodes, boolean dryRun) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
assert historyID != null;
assert nodes != null;
this.historyID = historyID;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ public class ClusterHealthRequest extends MasterNodeReadRequest<ClusterHealthReq
private String waitForNodes = "";
private Priority waitForEvents = null;

public ClusterHealthRequest() {}
public ClusterHealthRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public ClusterHealthRequest(String... indices) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.indices = indices;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
public class GetFeatureUpgradeStatusRequest extends MasterNodeRequest<GetFeatureUpgradeStatusRequest> {

public GetFeatureUpgradeStatusRequest() {
super();
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public GetFeatureUpgradeStatusRequest(StreamInput in) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
public class PostFeatureUpgradeRequest extends MasterNodeRequest<PostFeatureUpgradeRequest> {

public PostFeatureUpgradeRequest() {
super();
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public PostFeatureUpgradeRequest(StreamInput in) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public class PrevalidateNodeRemovalRequest extends MasterNodeReadRequest<Prevali
private TimeValue timeout = TimeValue.timeValueSeconds(30);

private PrevalidateNodeRemovalRequest(Builder builder) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.names = builder.names;
this.ids = builder.ids;
this.externalIds = builder.externalIds;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@ public class CleanupRepositoryRequest extends AcknowledgedRequest<CleanupReposit
private String repository;

public CleanupRepositoryRequest(String repository) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
this.repository = repository;
}

public CleanupRepositoryRequest(StreamInput in) throws IOException {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
repository = in.readString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,17 @@ public DeleteRepositoryRequest(StreamInput in) throws IOException {
name = in.readString();
}

public DeleteRepositoryRequest() {}
public DeleteRepositoryRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
}

/**
* Constructs a new unregister repository request with the provided name.
*
* @param name name of the repository
*/
public DeleteRepositoryRequest(String name) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
this.name = name;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ public class GetRepositoriesRequest extends MasterNodeReadRequest<GetRepositorie

private String[] repositories = Strings.EMPTY_ARRAY;

public GetRepositoriesRequest() {}
public GetRepositoriesRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

/**
* Constructs a new get repositories request with a list of repositories.
Expand All @@ -36,6 +38,7 @@ public GetRepositoriesRequest() {}
* @param repositories list of repositories
*/
public GetRepositoriesRequest(String[] repositories) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.repositories = repositories;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,15 @@ public PutRepositoryRequest(StreamInput in) throws IOException {
verify = in.readBoolean();
}

public PutRepositoryRequest() {}
public PutRepositoryRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
}

/**
* Constructs a new put repository request with the provided name.
*/
public PutRepositoryRequest(String name) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
this.name = name;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,17 @@ public VerifyRepositoryRequest(StreamInput in) throws IOException {
name = in.readString();
}

public VerifyRepositoryRequest() {}
public VerifyRepositoryRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
}

/**
* Constructs a new unregister repository request with the provided name.
*
* @param name name of the repository
*/
public VerifyRepositoryRequest(String name) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
this.name = name;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ public ClusterRerouteRequest(StreamInput in) throws IOException {
retryFailed = in.readBoolean();
}

public ClusterRerouteRequest() {}
public ClusterRerouteRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
}

/**
* Adds allocation commands to be applied to the cluster. Note, can be empty, in which case
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public ClusterGetSettingsAction() {
* Request to retrieve the cluster settings
*/
public static class Request extends MasterNodeReadRequest<Request> {
public Request() {}
public Request() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public Request(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ public ClusterUpdateSettingsRequest(StreamInput in) throws IOException {
persistentSettings = readSettingsFromStream(in);
}

public ClusterUpdateSettingsRequest() {}
public ClusterUpdateSettingsRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
}

@Override
public ActionRequestValidationException validate() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@ public final class ClusterSearchShardsRequest extends MasterNodeReadRequest<Clus
private String preference;
private IndicesOptions indicesOptions = IndicesOptions.lenientExpandOpen();

public ClusterSearchShardsRequest() {}
public ClusterSearchShardsRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public ClusterSearchShardsRequest(String... indices) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
indices(indices);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public CloneSnapshotRequest(StreamInput in) throws IOException {
* @param indices indices to clone from source to target
*/
public CloneSnapshotRequest(String repository, String source, String target, String[] indices) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.repository = repository;
this.source = source;
this.target = target;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ public class CreateSnapshotRequest extends MasterNodeRequest<CreateSnapshotReque
@Nullable
private Map<String, Object> userMetadata;

public CreateSnapshotRequest() {}
public CreateSnapshotRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

/**
* Constructs a new put repository request with the provided snapshot and repository names
Expand All @@ -87,6 +89,7 @@ public CreateSnapshotRequest() {}
* @param snapshot snapshot name
*/
public CreateSnapshotRequest(String repository, String snapshot) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.snapshot = snapshot;
this.repository = repository;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class DeleteSnapshotRequest extends MasterNodeRequest<DeleteSnapshotReque
* @param snapshots snapshot names
*/
public DeleteSnapshotRequest(String repository, String... snapshots) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.repository = repository;
this.snapshots = snapshots;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public class GetSnapshottableFeaturesRequest extends MasterNodeRequest<GetSnapsh

public GetSnapshottableFeaturesRequest() {

super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

public GetSnapshottableFeaturesRequest(StreamInput in) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ public static ResetFeatureStateRequest fromStream(StreamInput in) throws IOExcep
return new ResetFeatureStateRequest(in);
}

public ResetFeatureStateRequest() {}
public ResetFeatureStateRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

private ResetFeatureStateRequest(StreamInput in) throws IOException {
super(in);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>

private boolean includeIndexNames = true;

public GetSnapshotsRequest() {}
public GetSnapshotsRequest() {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
}

/**
* Constructs a new get snapshots request with given repository names and list of snapshots
Expand All @@ -85,6 +87,7 @@ public GetSnapshotsRequest() {}
* @param snapshots list of snapshots
*/
public GetSnapshotsRequest(String[] repositories, String[] snapshots) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.repositories = repositories;
this.snapshots = snapshots;
}
Expand All @@ -95,6 +98,7 @@ public GetSnapshotsRequest(String[] repositories, String[] snapshots) {
* @param repositories repository names
*/
public GetSnapshotsRequest(String... repositories) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
this.repositories = repositories;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class GetShardSnapshotRequest extends MasterNodeRequest<GetShardSnapshotR
private final ShardId shardId;

GetShardSnapshotRequest(List<String> repositories, ShardId shardId) {
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
assert repositories.isEmpty() == false;
assert repositories.stream().noneMatch(Objects::isNull);
assert repositories.size() == 1 || repositories.stream().noneMatch(repo -> repo.equals(ALL_REPOSITORIES));
Expand Down

0 comments on commit 0b2e558

Please sign in to comment.