-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add state query param to Get snapshots API #123618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
18fd911
d80ec19
712138f
a5bc52e
91ae28b
d9f8faf
f6f0b54
c2b19d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ | |
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.EnumSet; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
@@ -69,6 +70,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 { | ||
|
||
|
@@ -633,6 +635,55 @@ public void testRetrievingSnapshotsWhenRepositoryIsMissing() throws Exception { | |
expectThrows(RepositoryMissingException.class, multiRepoFuture::actionGet); | ||
} | ||
|
||
public void testFilterByState() throws Exception { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also add this feature to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
// Create a successful snapshot | ||
String successSnapshot = "snapshot-success"; | ||
createFullSnapshot(repoName, successSnapshot); | ||
|
||
// Fetch snapshots with state=SUCCESS | ||
GetSnapshotsResponse responseSuccess = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) | ||
.setState(EnumSet.of(SnapshotState.SUCCESS)) | ||
.get(); | ||
assertThat(responseSuccess.getSnapshots(), hasSize(1)); | ||
assertThat(responseSuccess.getSnapshots().get(0).state(), is(SnapshotState.SUCCESS)); | ||
|
||
// Create a snapshot in progress | ||
String inProgressSnapshot = "snapshot-in-progress"; | ||
blockAllDataNodes(repoName); | ||
startFullSnapshot(repoName, inProgressSnapshot); | ||
awaitNumberOfSnapshotsInProgress(1); | ||
|
||
// Fetch snapshots with state=IN_PROGRESS | ||
GetSnapshotsResponse responseInProgress = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) | ||
.setState(EnumSet.of(SnapshotState.IN_PROGRESS)) | ||
.get(); | ||
assertThat(responseInProgress.getSnapshots(), hasSize(1)); | ||
assertThat(responseInProgress.getSnapshots().get(0).state(), is(SnapshotState.IN_PROGRESS)); | ||
|
||
// Fetch snapshots with multiple states (SUCCESS, IN_PROGRESS) | ||
GetSnapshotsResponse responseMultipleStates = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) | ||
.setState(EnumSet.of(SnapshotState.SUCCESS, SnapshotState.IN_PROGRESS)) | ||
.get(); | ||
assertThat(responseMultipleStates.getSnapshots(), hasSize(2)); | ||
assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.SUCCESS)); | ||
assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.IN_PROGRESS)); | ||
|
||
// Fetch all snapshots (without state) | ||
GetSnapshotsResponse responseAll = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get(); | ||
assertThat(responseAll.getSnapshots(), hasSize(2)); | ||
|
||
// Fetch snapshots with an invalid state | ||
IllegalArgumentException e = expectThrows( | ||
IllegalArgumentException.class, | ||
() -> clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setState(EnumSet.of(SnapshotState.of("FOO"))).get() | ||
); | ||
assertThat(e.getMessage(), containsString("Unknown state name [FOO]")); | ||
} | ||
|
||
// 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 | ||
|
@@ -912,9 +963,15 @@ public void testAllFeatures() { | |
// INDICES and by SHARDS. The actual sorting behaviour for these cases is tested elsewhere, here we're just checking that sorting | ||
// interacts correctly with the other parameters to the API. | ||
|
||
final EnumSet<SnapshotState> state = EnumSet.of(randomFrom(SnapshotState.values())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we test a random nonempty subset rather than just a singleton? |
||
// Note: The selected state may not match any existing snapshots. | ||
// The actual filtering behaviour for such cases is tested in the dedicated test. | ||
// Here we're just checking that state interacts correctly with the other parameters to the API. | ||
|
||
// compute the ordered sequence of snapshots which match the repository/snapshot name filters and SLM policy filter | ||
final var selectedSnapshots = snapshotInfos.stream() | ||
.filter(snapshotInfoPredicate) | ||
.filter(s -> state.contains(s.state())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should fold the state filter into |
||
.sorted(sortKey.getSnapshotInfoComparator(order)) | ||
.toList(); | ||
|
||
|
@@ -923,7 +980,8 @@ public void testAllFeatures() { | |
) | ||
// apply sorting params | ||
.sort(sortKey) | ||
.order(order); | ||
.order(order) | ||
.state(state); | ||
|
||
// sometimes use ?from_sort_value to skip some items; note that snapshots skipped in this way are subtracted from | ||
// GetSnapshotsResponse.totalCount whereas snapshots skipped by ?after and ?offset are not | ||
|
@@ -1010,7 +1068,8 @@ public void testAllFeatures() { | |
.sort(sortKey) | ||
.order(order) | ||
.size(nextSize) | ||
.after(SnapshotSortKey.decodeAfterQueryParam(nextRequestAfter)); | ||
.after(SnapshotSortKey.decodeAfterQueryParam(nextRequestAfter)) | ||
.state(state); | ||
final GetSnapshotsResponse nextResponse = safeAwait(l -> client().execute(TransportGetSnapshotsAction.TYPE, nextRequest, l)); | ||
|
||
assertEquals( | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,12 +19,14 @@ | |||||
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; | ||||||
|
||||||
import java.io.IOException; | ||||||
import java.util.Arrays; | ||||||
import java.util.EnumSet; | ||||||
import java.util.Map; | ||||||
|
||||||
import static org.elasticsearch.action.ValidateActions.addValidationError; | ||||||
|
@@ -39,6 +41,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; | ||||||
|
||||||
|
@@ -77,6 +80,8 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest> | |||||
|
||||||
private boolean includeIndexNames = true; | ||||||
|
||||||
private EnumSet<SnapshotState> state = EnumSet.noneOf(SnapshotState.class); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
public GetSnapshotsRequest(TimeValue masterNodeTimeout) { | ||||||
super(masterNodeTimeout); | ||||||
} | ||||||
|
@@ -118,6 +123,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.readEnumSet(SnapshotState.class); | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should avoid |
||||||
} | ||||||
|
||||||
@Override | ||||||
|
@@ -137,6 +145,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.writeEnumSet(state); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a mixed-version cluster where not all nodes understand this feature we shouldn't permit users to use it rather than just silently ignoring it as done here. IOW we need an Specifically, we need to introduce a |
||||||
} | ||||||
} | ||||||
|
||||||
@Override | ||||||
|
@@ -342,6 +353,15 @@ public boolean verbose() { | |||||
return verbose; | ||||||
} | ||||||
|
||||||
public EnumSet<SnapshotState> state() { | ||||||
return state; | ||||||
} | ||||||
|
||||||
public GetSnapshotsRequest state(EnumSet<SnapshotState> state) { | ||||||
this.state = state; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validation nit
Suggested change
|
||||||
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); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,9 @@ | |||||
import org.elasticsearch.core.Nullable; | ||||||
import org.elasticsearch.core.TimeValue; | ||||||
import org.elasticsearch.search.sort.SortOrder; | ||||||
import org.elasticsearch.snapshots.SnapshotState; | ||||||
|
||||||
import java.util.EnumSet; | ||||||
|
||||||
/** | ||||||
* Get snapshots request builder | ||||||
|
@@ -150,4 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) { | |||||
|
||||||
} | ||||||
|
||||||
public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming nit
Suggested change
|
||||||
request.state(state); | ||||||
return this; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -45,6 +45,7 @@ | |||||
import org.elasticsearch.snapshots.SnapshotId; | ||||||
import org.elasticsearch.snapshots.SnapshotInfo; | ||||||
import org.elasticsearch.snapshots.SnapshotMissingException; | ||||||
import org.elasticsearch.snapshots.SnapshotState; | ||||||
import org.elasticsearch.snapshots.SnapshotsService; | ||||||
import org.elasticsearch.tasks.CancellableTask; | ||||||
import org.elasticsearch.tasks.Task; | ||||||
|
@@ -54,6 +55,7 @@ | |||||
|
||||||
import java.util.ArrayList; | ||||||
import java.util.Collections; | ||||||
import java.util.EnumSet; | ||||||
import java.util.HashMap; | ||||||
import java.util.HashSet; | ||||||
import java.util.Iterator; | ||||||
|
@@ -160,7 +162,8 @@ protected void masterOperation( | |||||
request.size(), | ||||||
SnapshotsInProgress.get(state), | ||||||
request.verbose(), | ||||||
request.includeIndexNames() | ||||||
request.includeIndexNames(), | ||||||
request.state() | ||||||
).runOperation(listener); | ||||||
} | ||||||
|
||||||
|
@@ -181,6 +184,7 @@ private class GetSnapshotsOperation { | |||||
private final SnapshotNamePredicate snapshotNamePredicate; | ||||||
private final SnapshotPredicates fromSortValuePredicates; | ||||||
private final Predicate<String> slmPolicyPredicate; | ||||||
private final EnumSet<SnapshotState> state; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming nit
Suggested change
|
||||||
|
||||||
// snapshot ordering/pagination | ||||||
private final SnapshotSortKey sortBy; | ||||||
|
@@ -224,7 +228,8 @@ private class GetSnapshotsOperation { | |||||
int size, | ||||||
SnapshotsInProgress snapshotsInProgress, | ||||||
boolean verbose, | ||||||
boolean indices | ||||||
boolean indices, | ||||||
EnumSet<SnapshotState> state | ||||||
) { | ||||||
this.cancellableTask = cancellableTask; | ||||||
this.repositories = repositories; | ||||||
|
@@ -237,6 +242,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); | ||||||
|
@@ -558,11 +564,16 @@ private boolean matchesPredicates(SnapshotId snapshotId, RepositoryData reposito | |||||
return false; | ||||||
} | ||||||
|
||||||
final var details = repositoryData.getSnapshotDetails(snapshotId); | ||||||
|
||||||
if (!state.isEmpty() && !state.contains(details.getSnapshotState())) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The style check will not like the unary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh also we need to handle the cases where |
||||||
return false; | ||||||
} | ||||||
|
||||||
if (slmPolicyPredicate == SlmPolicyPredicate.MATCH_ALL_POLICIES) { | ||||||
return true; | ||||||
} | ||||||
|
||||||
final var details = repositoryData.getSnapshotDetails(snapshotId); | ||||||
return details == null || details.getSlmPolicy() == null || slmPolicyPredicate.test(details.getSlmPolicy()); | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,11 @@ | |
import org.elasticsearch.rest.action.RestCancellableNodeClient; | ||
import org.elasticsearch.rest.action.RestRefCountedChunkedToXContentListener; | ||
import org.elasticsearch.search.sort.SortOrder; | ||
import org.elasticsearch.snapshots.SnapshotState; | ||
|
||
import java.io.IOException; | ||
import java.util.Arrays; | ||
import java.util.EnumSet; | ||
import java.util.List; | ||
import java.util.Set; | ||
|
||
|
@@ -82,6 +85,14 @@ 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())); | ||
|
||
final String stateString = request.param("state"); | ||
if (stateString == null || stateString.isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: suggest using |
||
getSnapshotsRequest.state(EnumSet.noneOf(SnapshotState.class)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest defaulting to |
||
} else { | ||
getSnapshotsRequest.state(EnumSet.copyOf(Arrays.stream(stateString.split(",")).map(SnapshotState::of).toList())); | ||
} | ||
|
||
return channel -> new RestCancellableNodeClient(client, request.getHttpChannel()).admin() | ||
.cluster() | ||
.getSnapshots(getSnapshotsRequest, new RestRefCountedChunkedToXContentListener<>(channel)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,4 +89,21 @@ public static SnapshotState fromValue(byte value) { | |
default -> throw new IllegalArgumentException("No snapshot state for value [" + value + "]"); | ||
}; | ||
} | ||
|
||
/** | ||
* Generate snapshot state from a string (case-insensitive) | ||
* | ||
* @param name the state name | ||
* @return state | ||
*/ | ||
public static SnapshotState of(String name) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming nit: maybe Also could we not use the built-in |
||
return switch (name.toUpperCase()) { | ||
case "IN_PROGRESS" -> IN_PROGRESS; | ||
case "SUCCESS" -> SUCCESS; | ||
case "FAILED" -> FAILED; | ||
case "PARTIAL" -> PARTIAL; | ||
case "INCOMPATIBLE" -> INCOMPATIBLE; | ||
default -> throw new IllegalArgumentException("Unknown state name [" + name + "]"); | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
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
andSUCCESS,PARTIAL
both return the expected snapshots.