Add state query param to Get snapshots API #123618

ElenaStoeva · 2025-02-27T16:00:11Z

This PR introduces a new state query parameter for the Get Snapshots API, allowing users to filter snapshots by state. The parameter accepts comma-separated values for states: SUCCESS, IN_PROGRESS, FAILED, PARTIAL, INCOMPATIBLE (case-insensitive).

🎯 Motivation & Use Case

This is my mini-project for the AppEx team’s On-Week, where I aimed to explore contributing to Elasticsearch while addressing several Kibana issues.

In Kibana's Snapshot Restore page, multiple open issues could be resolved by adding this parameter:

1️⃣ Fixing incorrect selection of last successful managed snapshot

Helps correctly identify the last successful managed snapshot, ensuring delete actions are disabled for it.
Resolves bugs kibana#158548 and kibana#153107

2️⃣ Enhancing the Snapshots table:

Enables filtering/sorting snapshots by their state, improving usability.
Addresses issues kibana#137432 and kibana#202883

✅ Summary of Changes

Added state as an optional query parameter to GET _snapshot/{repository}/_all
Updated filtering logic in TransportGetSnapshotsAction
Added validation of the parameter value
Added integration tests in GetSnapshotsIT.java

github-actions · 2025-02-27T16:00:22Z

Documentation preview:

✨ Changed pages

ElenaStoeva · 2025-02-27T16:02:38Z

@elasticsearchmachine test this

elasticsearchmachine · 2025-02-28T18:12:43Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

elasticsearchmachine · 2025-02-28T18:13:22Z

Pinging @elastic/es-distributed-obsolete (Team:Distributed (Obsolete))

DaveCTurner · 2025-02-28T18:20:57Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

@@ -571,6 +575,10 @@ private boolean matchesPredicates(SnapshotInfo snapshotInfo) {
                return false;
            }

+            if (state != null) {
+                return state.equalsIgnoreCase(snapshotInfo.state().toString());


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.

Sure! Changed this in d9f8faf.

DaveCTurner · 2025-02-28T18:21:53Z

.../src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java

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

    private boolean includeIndexNames = true;

+    private String state;


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.

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.

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.

DaveCTurner · 2025-02-28T18:23:48Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

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

+    public void testFilterByState() throws Exception {


Could we also add this feature to testAllFeatures?

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

ElenaStoeva · 2025-03-05T14:21:07Z

Many thanks for the review @DaveCTurner! I addressed your comments, please let me know if you have any additional suggestions.

DaveCTurner

👍 API makes more sense now. I did another pass and left more comments

DaveCTurner · 2025-03-06T15:20:03Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

+
+        final String stateString = request.param("state");
+        if (stateString == null || stateString.isEmpty()) {
+            getSnapshotsRequest.state(EnumSet.noneOf(SnapshotState.class));


Suggest defaulting to EnumSet.allOf rather than having a special case for the empty set lower down. We could reasonably reject the empty set in org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest#validate I suppose since otherwise that'll just filter out everything.

DaveCTurner · 2025-03-06T15:20:42Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java

@@ -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()) {


nit: suggest using org.elasticsearch.common.Strings#hasLength(java.lang.String) here

DaveCTurner · 2025-03-06T15:21:23Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

@@ -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())) {


The style check will not like the unary ! operators here, you need to say state.contains(details.getSnapshotState()) == false instead.

DaveCTurner · 2025-03-06T15:21:37Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

@@ -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;


naming nit

Suggested change

private final EnumSet<SnapshotState> state;

private final EnumSet<SnapshotState> states;

DaveCTurner · 2025-03-06T15:21:57Z

...in/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java

@@ -150,4 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) {

    }

+    public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) {


naming nit

Suggested change

public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) {

public GetSnapshotsRequestBuilder setStates(EnumSet<SnapshotState> state) {

DaveCTurner · 2025-03-06T15:27:48Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

@@ -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())) {


Oh also we need to handle the cases where details or details.getSnapshotState() are null. In that case, we cannot do the pre-flight check, we will need to retrieve the SnapshotInfo and then check that in the other matchesPredicates() method

DaveCTurner · 2025-03-06T15:29:24Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java

+     * @param name the state name
+     * @return state
+     */
+    public static SnapshotState of(String name) {


Naming nit: maybe fromString rather than of? As written here it makes sense if you call the method directly but it's a little awkward to read .map(SnapshotState::of), I'd prefer .map(SnapshotState::fromString).

Also could we not use the built-in valueOf()?

DaveCTurner · 2025-03-06T15:31:55Z

rest-api-spec/src/main/resources/rest-api-spec/api/snapshot.get.json

@@ -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": {


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.

DaveCTurner · 2025-03-06T15:32:46Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

@@ -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()));


Could we test a random nonempty subset rather than just a singleton?

DaveCTurner · 2025-03-06T15:33:41Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

        // 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()))


I think we should fold the state filter into snapshotInfoPredicate, since we're really implementing it alongside the SLM policy check etc.

ElenaStoeva · 2025-03-31T09:16:05Z

Thanks for the review, @DaveCTurner, and apologies for the delay in addressing the feedback. I've been focused on Kibana-related tasks for my team and, unfortunately, haven’t had a chance to revisit this PR. @JeremyDahlgren reached out and offered to take it over, so he’ll be picking it up from here. I really appreciate your time and input!

ElenaStoeva · 2025-06-17T10:28:27Z

Closing in favor of #128635

Add state query param to Get snapshots API

18fd911

ElenaStoeva added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.1.0 labels Feb 27, 2025

ElenaStoeva self-assigned this Feb 27, 2025

elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Feb 27, 2025

ElenaStoeva and others added 4 commits February 28, 2025 13:54

Fix checkstyle error

d80ec19

Resolve merge conflicts

712138f

Merge branch 'main' into feature/97446

a5bc52e

Resolve bwc test errors

91ae28b

ElenaStoeva marked this pull request as ready for review February 28, 2025 18:12

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Feb 28, 2025

ElenaStoeva added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. and removed Team:Distributed Coordination Meta label for Distributed Coordination team labels Feb 28, 2025

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Feb 28, 2025

DaveCTurner reviewed Feb 28, 2025

View reviewed changes

ElenaStoeva and others added 2 commits March 5, 2025 13:55

Address feedback

d9f8faf

Merge branch 'main' into feature/97446

f6f0b54

Fix style errors

c2b19d6

DaveCTurner reviewed Mar 6, 2025

View reviewed changes

ElenaStoeva assigned JeremyDahlgren Mar 31, 2025

JeremyDahlgren mentioned this pull request May 29, 2025

Add state query param to GET snapshots API #128635

Merged

ElenaStoeva closed this Jun 17, 2025

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

		private boolean includeIndexNames = true;

		private String state;

	private final EnumSet<SnapshotState> state;
	private final EnumSet<SnapshotState> states;

		@@ -150,4 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) {

		}

		public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) {

Add state query param to Get snapshots API #123618

Add state query param to Get snapshots API #123618

Uh oh!

Conversation

ElenaStoeva commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎯 Motivation & Use Case

1️⃣ Fixing incorrect selection of last successful managed snapshot

2️⃣ Enhancing the Snapshots table:

✅ Summary of Changes

Uh oh!

github-actions bot commented Feb 27, 2025

Uh oh!

ElenaStoeva commented Feb 27, 2025

Uh oh!

elasticsearchmachine commented Feb 28, 2025

Uh oh!

elasticsearchmachine commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElenaStoeva commented Mar 5, 2025

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElenaStoeva commented Mar 31, 2025

Uh oh!

ElenaStoeva commented Jun 17, 2025

Uh oh!

Uh oh!

ElenaStoeva commented Feb 27, 2025 •

edited

Loading