Skip to content

Commit

Permalink
Make master-node timeout less implicit
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 elastic#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 elastic#107984
  • Loading branch information
DaveCTurner committed May 8, 2024
1 parent e7350dc commit d2c5a4a
Show file tree
Hide file tree
Showing 214 changed files with 499 additions and 165 deletions.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 d2c5a4a

Please sign in to comment.