Skip to content
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

Remove the failures field from snapshot responses #114496

Merged
merged 16 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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]
)
);

Expand All @@ -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"));
Expand Down
Original file line number Diff 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
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ static TransportVersion def(int id) {
public static final TransportVersion COHERE_BIT_EMBEDDING_TYPE_SUPPORT_ADDED_BACKPORT_8_X = def(8_840_0_01);
public static final TransportVersion ELASTICSEARCH_9_0 = def(9_000_0_00);
public static final TransportVersion COHERE_BIT_EMBEDDING_TYPE_SUPPORT_ADDED = def(9_001_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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand All @@ -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;
}
Expand All @@ -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);
Expand All @@ -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);
}
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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++) {
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ public void testSlmPolicyExecutedAfterStep() {
SnapshotState.SUCCESS
)
),
Map.of(),
null,
0,
0
Expand Down Expand Up @@ -202,7 +201,6 @@ public void testIndexNotBackedUpYet() {
SnapshotState.SUCCESS
)
),
Map.of(),
null,
0,
0
Expand Down