Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@
"verbose":{
"type":"boolean",
"description":"Whether to show verbose snapshot info or only show the basic info found in the repository index blob"
},
"state": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add some simple smoke tests to rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml just to check that we are parsing the parameter ok. No need for anything exhaustive, just check that e.g. SUCCESS and SUCCESS,PARTIAL both return the expected snapshots.

"type": "string",
"description": "Filter snapshots by state. Valid values are 'SUCCESS', 'IN_PROGRESS', 'FAILED', 'PARTIAL', or 'INCOMPATIBLE'."
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.in;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.core.StringContains.containsString;

public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase {

Expand Down Expand Up @@ -633,6 +634,57 @@ public void testRetrievingSnapshotsWhenRepositoryIsMissing() throws Exception {
expectThrows(RepositoryMissingException.class, multiRepoFuture::actionGet);
}

public void testFilterByState() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add this feature to testAllFeatures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added some checks there as well. Let me know if I need to add any other tests.

final String repoName = "test-repo";
final Path repoPath = randomRepoPath();
createRepository(repoName, "mock", repoPath);
maybeInitWithOldSnapshotVersion(repoName, repoPath);

// Create a successful snapshot
String successSnapshot = "snapshot-success";
createFullSnapshot(repoName, successSnapshot);

// Create a snapshot in progress
String inProgressSnapshot = "snapshot-in-progress";
blockAllDataNodes(repoName);
startFullSnapshot(repoName, inProgressSnapshot);
awaitNumberOfSnapshotsInProgress(1);
awaitClusterState(
state -> SnapshotsInProgress.get(state)
.asStream()
.flatMap(s -> s.shards().entrySet().stream())
.allMatch(
e -> e.getKey().getIndexName().equals("test-index-1") == false
|| e.getValue().state() == SnapshotsInProgress.ShardState.SUCCESS
)
);

// Fetch all snapshots
GetSnapshotsResponse responseAll = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get();
assertThat(responseAll.getSnapshots(), hasSize(2));

// Fetch snapshots with state=SUCCESS
GetSnapshotsResponse responseSuccess = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName)
.setState(String.valueOf(SnapshotState.SUCCESS))
.get();
assertThat(responseSuccess.getSnapshots(), hasSize(1));
assertThat(responseSuccess.getSnapshots().get(0).state(), is(SnapshotState.SUCCESS));

// Fetch snapshots with state=IN_PROGRESS
GetSnapshotsResponse responseInProgress = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName)
.setState(String.valueOf(SnapshotState.IN_PROGRESS))
.get();
assertThat(responseInProgress.getSnapshots(), hasSize(1));
assertThat(responseInProgress.getSnapshots().get(0).state(), is(SnapshotState.IN_PROGRESS));

// Fetch snapshots with invalid state
IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setState("INVALID_STATE").get()
);
assertThat(e.getMessage(), containsString("state must be SUCCESS, IN_PROGRESS, FAILED, PARTIAL, or INCOMPATIBLE"));
}

// Create a snapshot that is guaranteed to have a unique start time and duration for tests around ordering by either.
// Don't use this with more than 3 snapshots on platforms with low-resolution clocks as the durations could always collide there
// causing an infinite loop
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ static TransportVersion def(int id) {
public static final TransportVersion ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS = def(9_016_0_00);
public static final TransportVersion ESQL_DRIVER_NODE_DESCRIPTION = def(9_017_0_00);
public static final TransportVersion MULTI_PROJECT = def(9_018_0_00);
public static final TransportVersion STATE_PARAM_GET_SNAPSHOT = def(9_019_0_00);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.search.sort.SortOrder;
import org.elasticsearch.snapshots.SnapshotState;
import org.elasticsearch.tasks.CancellableTask;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.tasks.TaskId;
Expand All @@ -39,6 +40,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>
public static final boolean DEFAULT_VERBOSE_MODE = true;

private static final TransportVersion INDICES_FLAG_VERSION = TransportVersions.V_8_3_0;
private static final TransportVersion STATE_FLAG_VERSION = TransportVersions.STATE_PARAM_GET_SNAPSHOT;

public static final int NO_LIMIT = -1;

Expand Down Expand Up @@ -77,6 +79,8 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>

private boolean includeIndexNames = true;

private String state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this a (nullable) SnapshotState rather than a string? Also please mark as @Nullable with a comment saying what the null value means.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also are you sure you only want to pick one state? What about a comma-separated list of values perhaps? For instance SUCCESS,PARTIAL would give you all the complete snapshots. If so, maybe an EnumSet<SnapshotState> instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I think it would be useful to make it an EnumSet<SnapshotState> so that we can filter by multiple states. Added the change with d9f8faf.


public GetSnapshotsRequest(TimeValue masterNodeTimeout) {
super(masterNodeTimeout);
}
Expand Down Expand Up @@ -118,6 +122,9 @@ public GetSnapshotsRequest(StreamInput in) throws IOException {
if (in.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) {
includeIndexNames = in.readBoolean();
}
if (in.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) {
state = in.readOptionalString();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid state being null here, by setting it to EnumSet.allOf(SnapshotState.class) on the else branch

}

@Override
Expand All @@ -137,6 +144,9 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) {
out.writeBoolean(includeIndexNames);
}
if (out.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) {
out.writeOptionalString(state);
}
}

@Override
Expand Down Expand Up @@ -177,6 +187,12 @@ public ActionRequestValidationException validate() {
} else if (after != null && fromSortValue != null) {
validationException = addValidationError("can't use after and from_sort_value simultaneously", validationException);
}
if (state != null && Arrays.stream(SnapshotState.values()).noneMatch(s -> s.name().equalsIgnoreCase(state))) {
validationException = addValidationError(
"state must be SUCCESS, IN_PROGRESS, FAILED, PARTIAL, or INCOMPATIBLE",
validationException
);
}
return validationException;
}

Expand Down Expand Up @@ -342,6 +358,15 @@ public boolean verbose() {
return verbose;
}

public String state() {
return state;
}

public GetSnapshotsRequest state(String state) {
this.state = state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validation nit

Suggested change
this.state = state;
this.state = Objects.requireNonNull(state);

return this;
}

@Override
public Task createTask(long id, String type, String action, TaskId parentTaskId, Map<String, String> headers) {
return new CancellableTask(id, type, action, getDescription(), parentTaskId, headers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) {

}

public GetSnapshotsRequestBuilder setState(String state) {
request.state(state);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ protected void masterOperation(
request.size(),
SnapshotsInProgress.get(state),
request.verbose(),
request.includeIndexNames()
request.includeIndexNames(),
request.state()
).runOperation(listener);
}

Expand All @@ -181,6 +182,7 @@ private class GetSnapshotsOperation {
private final SnapshotNamePredicate snapshotNamePredicate;
private final SnapshotPredicates fromSortValuePredicates;
private final Predicate<String> slmPolicyPredicate;
private final String state;

// snapshot ordering/pagination
private final SnapshotSortKey sortBy;
Expand Down Expand Up @@ -224,7 +226,8 @@ private class GetSnapshotsOperation {
int size,
SnapshotsInProgress snapshotsInProgress,
boolean verbose,
boolean indices
boolean indices,
String state
) {
this.cancellableTask = cancellableTask;
this.repositories = repositories;
Expand All @@ -237,6 +240,7 @@ private class GetSnapshotsOperation {
this.snapshotsInProgress = snapshotsInProgress;
this.verbose = verbose;
this.indices = indices;
this.state = state;

this.snapshotNamePredicate = SnapshotNamePredicate.forSnapshots(ignoreUnavailable, snapshots);
this.fromSortValuePredicates = SnapshotPredicates.forFromSortValue(fromSortValue, sortBy, order);
Expand Down Expand Up @@ -571,6 +575,10 @@ private boolean matchesPredicates(SnapshotInfo snapshotInfo) {
return false;
}

if (state != null) {
return state.equalsIgnoreCase(snapshotInfo.state().toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also do this as a pre-flight check (in the other matchesPredicates method)? No need to do the expensive SnapshotInfo read if we can help it, and most of the time this info is going to be available in RepositoryDetails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Changed this in d9f8faf.

}

if (slmPolicyPredicate == SlmPolicyPredicate.MATCH_ALL_POLICIES) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
final SortOrder order = SortOrder.fromString(request.param("order", getSnapshotsRequest.order().toString()));
getSnapshotsRequest.order(order);
getSnapshotsRequest.includeIndexNames(request.paramAsBoolean(INDEX_NAMES_XCONTENT_PARAM, getSnapshotsRequest.includeIndexNames()));
getSnapshotsRequest.state(request.param("state"));
return channel -> new RestCancellableNodeClient(client, request.getHttpChannel()).admin()
.cluster()
.getSnapshots(getSnapshotsRequest, new RestRefCountedChunkedToXContentListener<>(channel));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ public void testValidateParameters() {
final ActionRequestValidationException e = request.validate();
assertThat(e.getMessage(), containsString("can't use slm policy filter with verbose=false"));
}
{
final GetSnapshotsRequest request = new GetSnapshotsRequest(TEST_REQUEST_TIMEOUT, "repo", "snapshot").state("foo")
.verbose(false);
final ActionRequestValidationException e = request.validate();
assertThat(e.getMessage(), containsString("state must be SUCCESS, IN_PROGRESS, FAILED, PARTIAL, or INCOMPATIBLE"));
}
}

public void testGetDescription() {
Expand Down