[9.0] Remove the failures field from snapshot responses (#114496) (#…

…121770) Backports #114496 to 9.0 > Failure handling for snapshots was made stricter in #107191 (8.15), so this field is always empty since then. Clients don't need to check it anymore for failure handling, we can remove it from API responses in 9.0
elastic · Feb 5, 2025 · 83eb627 · 83eb627
1 parent 5bb6a3a
commit 83eb627
Show file tree

Hide file tree

Showing 8 changed files with 17 additions and 94 deletions.
diff --git a/...-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java b/...-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java
@@ -10,7 +10,6 @@
 package org.elasticsearch.http.snapshots;
 
 import org.apache.http.client.methods.HttpGet;
-import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.ActionFuture;
 import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
 import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest;
@@ -37,7 +36,6 @@
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -516,10 +514,9 @@ private static GetSnapshotsResponse sortedWithLimit(
         true,
         (args) -> new GetSnapshotsResponse(
             (List<SnapshotInfo>) args[0],
-            (Map<String, ElasticsearchException>) args[1],
-            (String) args[2],
-            args[3] == null ? UNKNOWN_COUNT : (int) args[3],
-            args[4] == null ? UNKNOWN_COUNT : (int) args[4]
+            (String) args[1],
+            args[2] == null ? UNKNOWN_COUNT : (int) args[2],
+            args[3] == null ? UNKNOWN_COUNT : (int) args[3]
         )
     );
 
@@ -529,11 +526,6 @@ private static GetSnapshotsResponse sortedWithLimit(
             (p, c) -> SnapshotInfoUtils.snapshotInfoFromXContent(p),
             new ParseField("snapshots")
         );
-        GET_SNAPSHOT_PARSER.declareObject(
-            ConstructingObjectParser.optionalConstructorArg(),
-            (p, c) -> p.map(HashMap::new, ElasticsearchException::fromXContent),
-            new ParseField("failures")
-        );
         GET_SNAPSHOT_PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), new ParseField("next"));
         GET_SNAPSHOT_PARSER.declareIntOrNull(ConstructingObjectParser.optionalConstructorArg(), UNKNOWN_COUNT, new ParseField("total"));
         GET_SNAPSHOT_PARSER.declareIntOrNull(ConstructingObjectParser.optionalConstructorArg(), UNKNOWN_COUNT, new ParseField("remaining"));

diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotStatusApisIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotStatusApisIT.java
@@ -316,7 +316,6 @@ public void testGetSnapshotsNoRepos() {
             .get();
 
         assertTrue(getSnapshotsResponse.getSnapshots().isEmpty());
-        assertTrue(getSnapshotsResponse.getFailures().isEmpty());
     }
 
     public void testGetSnapshotsMultipleRepos() throws Exception {

diff --git a/server/src/main/java/org/elasticsearch/TransportVersions.java b/server/src/main/java/org/elasticsearch/TransportVersions.java
@@ -173,7 +173,8 @@ static TransportVersion def(int id) {
     public static final TransportVersion INFERENCE_REQUEST_ADAPTIVE_RATE_LIMITING = def(8_839_0_00);
     public static final TransportVersion ML_INFERENCE_IBM_WATSONX_RERANK_ADDED = def(8_840_0_00);
     public static final TransportVersion ELASTICSEARCH_9_0 = def(9_000_0_00);
-    public static final TransportVersion TRANSPORT_STATS_HANDLING_TIME_REQUIRED = def(9_002_0_00);
+    public static final TransportVersion REMOVE_SNAPSHOT_FAILURES = def(9_002_0_00);
+    public static final TransportVersion TRANSPORT_STATS_HANDLING_TIME_REQUIRED = def(9_003_0_00);
 
     /*
      * STOP! READ THIS FIRST! No, really,

diff --git a/.../main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponse.java b/.../main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponse.java
@@ -9,20 +9,18 @@
 
 package org.elasticsearch.action.admin.cluster.snapshots.get;
 
-import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.TransportVersions;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.collect.Iterators;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.ChunkedToXContentObject;
 import org.elasticsearch.core.Nullable;
-import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.snapshots.SnapshotInfo;
 import org.elasticsearch.xcontent.ToXContent;
 
 import java.io.IOException;
-import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -35,33 +33,26 @@ public class GetSnapshotsResponse extends ActionResponse implements ChunkedToXCo
 
     private final List<SnapshotInfo> snapshots;
 
-    @UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION) // always empty, can be dropped
-    private final Map<String, ElasticsearchException> failures;
-
     @Nullable
     private final String next;
 
     private final int total;
 
     private final int remaining;
 
-    public GetSnapshotsResponse(
-        List<SnapshotInfo> snapshots,
-        Map<String, ElasticsearchException> failures,
-        @Nullable String next,
-        final int total,
-        final int remaining
-    ) {
+    public GetSnapshotsResponse(List<SnapshotInfo> snapshots, @Nullable String next, final int total, final int remaining) {
         this.snapshots = List.copyOf(snapshots);
-        this.failures = failures == null ? Map.of() : Map.copyOf(failures);
         this.next = next;
         this.total = total;
         this.remaining = remaining;
     }
 
     public GetSnapshotsResponse(StreamInput in) throws IOException {
         this.snapshots = in.readCollectionAsImmutableList(SnapshotInfo::readFrom);
-        this.failures = Collections.unmodifiableMap(in.readMap(StreamInput::readException));
+        if (in.getTransportVersion().before(TransportVersions.REMOVE_SNAPSHOT_FAILURES)) {
+            // Deprecated `failures` field
+            in.readMap(StreamInput::readException);
+        }
         this.next = in.readOptionalString();
         this.total = in.readVInt();
         this.remaining = in.readVInt();
@@ -76,25 +67,11 @@ public List<SnapshotInfo> getSnapshots() {
         return snapshots;
     }
 
-    /**
-     * Returns a map of repository name to {@link ElasticsearchException} for each unsuccessful response.
-     */
-    public Map<String, ElasticsearchException> getFailures() {
-        return failures;
-    }
-
     @Nullable
     public String next() {
         return next;
     }
 
-    /**
-     * Returns true if there is at least one failed response.
-     */
-    public boolean isFailed() {
-        return failures.isEmpty() == false;
-    }
-
     public int totalCount() {
         return total;
     }
@@ -106,7 +83,10 @@ public int remaining() {
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         out.writeCollection(snapshots);
-        out.writeMap(failures, StreamOutput::writeException);
+        if (out.getTransportVersion().before(TransportVersions.REMOVE_SNAPSHOT_FAILURES)) {
+            // Deprecated `failures` field
+            out.writeMap(Map.of(), StreamOutput::writeException);
+        }
         out.writeOptionalString(next);
         out.writeVInt(total);
         out.writeVInt(remaining);
@@ -120,18 +100,6 @@ public Iterator<ToXContent> toXContentChunked(ToXContent.Params params) {
             return b;
         }), Iterators.map(getSnapshots().iterator(), snapshotInfo -> snapshotInfo::toXContentExternal), Iterators.single((b, p) -> {
             b.endArray();
-            if (failures.isEmpty() == false) {
-                b.startObject("failures");
-                for (Map.Entry<String, ElasticsearchException> error : failures.entrySet()) {
-                    b.field(error.getKey(), (bb, pa) -> {
-                        bb.startObject();
-                        error.getValue().toXContent(bb, pa);
-                        bb.endObject();
-                        return bb;
-                    });
-                }
-                b.endObject();
-            }
             if (next != null) {
                 b.field("next", next);
             }
@@ -151,12 +119,12 @@ public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
         GetSnapshotsResponse that = (GetSnapshotsResponse) o;
-        return Objects.equals(snapshots, that.snapshots) && Objects.equals(failures, that.failures) && Objects.equals(next, that.next);
+        return Objects.equals(snapshots, that.snapshots) && Objects.equals(next, that.next);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(snapshots, failures, next);
+        return Objects.hash(snapshots, next);
     }
 
     @Override

diff --git a/...ava/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java b/...ava/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java
@@ -543,7 +543,6 @@ private GetSnapshotsResponse buildResponse() {
             }
             return new GetSnapshotsResponse(
                 snapshotInfos,
-                null,
                 remaining > 0 ? sortBy.encodeAfterQueryParam(snapshotInfos.get(snapshotInfos.size() - 1)) : null,
                 totalCount.get(),
                 remaining

diff --git a/server/src/main/java/org/elasticsearch/rest/action/cat/RestSnapshotAction.java b/server/src/main/java/org/elasticsearch/rest/action/cat/RestSnapshotAction.java
@@ -9,11 +9,9 @@
 
 package org.elasticsearch.rest.action.cat;
 
-import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest;
 import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse;
 import org.elasticsearch.client.internal.node.NodeClient;
-import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.Table;
 import org.elasticsearch.common.time.DateFormatter;
 import org.elasticsearch.core.TimeValue;
@@ -99,24 +97,6 @@ protected Table getTableWithHeader(RestRequest request) {
     private Table buildTable(RestRequest req, GetSnapshotsResponse getSnapshotsResponse) {
         Table table = getTableWithHeader(req);
 
-        if (getSnapshotsResponse.isFailed()) {
-            ElasticsearchException causes = null;
-
-            for (ElasticsearchException e : getSnapshotsResponse.getFailures().values()) {
-                if (causes == null) {
-                    causes = e;
-                } else {
-                    causes.addSuppressed(e);
-                }
-            }
-            throw new ElasticsearchException(
-                "Repositories ["
-                    + Strings.collectionToCommaDelimitedString(getSnapshotsResponse.getFailures().keySet())
-                    + "] failed to retrieve snapshots",
-                causes
-            );
-        }
-
         for (SnapshotInfo snapshotStatus : getSnapshotsResponse.getSnapshots()) {
             table.startRow();
 

diff --git a/.../java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponseTests.java b/.../java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponseTests.java
@@ -9,7 +9,6 @@
 
 package org.elasticsearch.action.admin.cluster.snapshots.get;
 
-import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.TransportVersion;
 import org.elasticsearch.common.UUIDs;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
@@ -31,14 +30,10 @@
 import java.util.Arrays;
 import java.util.Base64;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
-import static org.hamcrest.CoreMatchers.containsString;
-
 public class GetSnapshotsResponseTests extends ESTestCase {
     // We can not subclass AbstractSerializingTestCase because it
     // can only be used for instances with equals and hashCode
@@ -60,12 +55,6 @@ private GetSnapshotsResponse copyInstance(GetSnapshotsResponse instance) throws
     private void assertEqualInstances(GetSnapshotsResponse expectedInstance, GetSnapshotsResponse newInstance) {
         assertEquals(expectedInstance.getSnapshots(), newInstance.getSnapshots());
         assertEquals(expectedInstance.next(), newInstance.next());
-        assertEquals(expectedInstance.getFailures().keySet(), newInstance.getFailures().keySet());
-        for (Map.Entry<String, ElasticsearchException> expectedEntry : expectedInstance.getFailures().entrySet()) {
-            ElasticsearchException expectedException = expectedEntry.getValue();
-            ElasticsearchException newException = newInstance.getFailures().get(expectedEntry.getKey());
-            assertThat(newException.getMessage(), containsString(expectedException.getMessage()));
-        }
     }
 
     private List<SnapshotInfo> createSnapshotInfos(String repoName) {
@@ -99,7 +88,6 @@ private List<SnapshotInfo> createSnapshotInfos(String repoName) {
 
     private GetSnapshotsResponse createTestInstance() {
         Set<String> repositories = new HashSet<>();
-        Map<String, ElasticsearchException> failures = new HashMap<>();
         List<SnapshotInfo> responses = new ArrayList<>();
 
         for (int i = 0; i < randomIntBetween(0, 5); i++) {
@@ -111,12 +99,10 @@ private GetSnapshotsResponse createTestInstance() {
         for (int i = 0; i < randomIntBetween(0, 5); i++) {
             String repository = randomValueOtherThanMany(repositories::contains, () -> randomAlphaOfLength(10));
             repositories.add(repository);
-            failures.put(repository, new ElasticsearchException(randomAlphaOfLength(10)));
         }
 
         return new GetSnapshotsResponse(
             responses,
-            failures,
             randomBoolean()
                 ? Base64.getUrlEncoder()
                     .encodeToString(

diff --git a/.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForSnapshotStepTests.java b/.../plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForSnapshotStepTests.java
@@ -164,7 +164,6 @@ public void testSlmPolicyExecutedAfterStep() {
                     SnapshotState.SUCCESS
                 )
             ),
-            Map.of(),
             null,
             0,
             0
@@ -202,7 +201,6 @@ public void testIndexNotBackedUpYet() {
                     SnapshotState.SUCCESS
                 )
             ),
-            Map.of(),
             null,
             0,
             0
-Original file line number
+Diff line change
@@ Expand Up / @@ -316,7 +316,6 @@ public void testGetSnapshotsNoRepos() { @@
                 .get();
             assertTrue(getSnapshotsResponse.getSnapshots().isEmpty());
-            assertTrue(getSnapshotsResponse.getFailures().isEmpty());
         }
         public void testGetSnapshotsMultipleRepos() throws Exception {
@@ Expand Down @@