From 7855587dc11be1ab63e95bde83ec936899f34094 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 2 Apr 2021 11:29:49 -0400 Subject: [PATCH 01/16] Reset API should fail fast when there is an error --- .../snapshots/FeatureStateResetApiIT.java | 50 +++++++++++++++++++ .../elasticsearch/indices/SystemIndices.java | 2 +- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index 24bddd4958215..454d86e8ff116 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -8,10 +8,13 @@ package org.elasticsearch.snapshots; +import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; import org.elasticsearch.action.admin.indices.get.GetIndexResponse; +import org.elasticsearch.client.Client; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.indices.SystemIndexDescriptor; @@ -35,6 +38,7 @@ protected Collection> nodePlugins() { List> plugins = new ArrayList<>(super.nodePlugins()); plugins.add(SystemIndexTestPlugin.class); plugins.add(SecondSystemIndexTestPlugin.class); + plugins.add(EvilSystemIndexTestPlugin.class); return plugins; } @@ -65,6 +69,7 @@ public void testResetSystemIndices() throws Exception { assertThat(apiResponse.getItemList(), containsInAnyOrder( new ResetFeatureStateResponse.ResetFeatureStateStatus("SystemIndexTestPlugin", "SUCCESS"), new ResetFeatureStateResponse.ResetFeatureStateStatus("SecondSystemIndexTestPlugin", "SUCCESS"), + new ResetFeatureStateResponse.ResetFeatureStateStatus("EvilSystemIndexTestPlugin", "SUCCESS"), new ResetFeatureStateResponse.ResetFeatureStateStatus("tasks", "SUCCESS") )); @@ -94,6 +99,21 @@ public void testResetSystemIndices() throws Exception { assertThat(response.getIndices(), arrayContaining("my_index")); } + /** + * Evil test - what if the cleanup method fails? + */ + public void testFailFastOnStateCleanupFailure() throws Exception { + try { + EvilSystemIndexTestPlugin.beEvil = true; + Exception e1 = expectThrows(Exception.class, + () -> client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get()); + + assertThat(e1.getMessage(), containsString("problem!")); + } finally { + EvilSystemIndexTestPlugin.beEvil = false; + } + } + /** * A test plugin with patterns for system indices and associated indices. */ @@ -145,4 +165,34 @@ public String getFeatureDescription() { return "A second test plugin"; } } + + /** + * An evil test plugin to test failure cases. + */ + public static class EvilSystemIndexTestPlugin extends Plugin implements SystemIndexPlugin { + + private static boolean beEvil = false; + + @Override + public String getFeatureName() { + return "EvilSystemIndexTestPlugin"; + } + + @Override + public String getFeatureDescription() { + return "a plugin that can be very bad"; + } + + @Override + public void cleanUpFeature( + ClusterService clusterService, + Client client, + ActionListener listener) { + if (beEvil == false) { + listener.onResponse(new ResetFeatureStateResponse.ResetFeatureStateStatus(getFeatureName(), "SUCCESS")); + } else { + listener.onFailure(new Exception("problem!")); + } + } + } } diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index f1a5c852c6bfb..bb8d66808288f 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -358,7 +358,7 @@ public void onResponse(AcknowledgedResponse acknowledgedResponse) { @Override public void onFailure(Exception e) { - listener.onResponse(new ResetFeatureStateStatus(name, "FAILURE: " + e.getMessage())); + listener.onFailure(new Exception("Error resetting state for feature: " + name, e)); } }); } From 9a8f090f780664cae2f5d582af7827e226b44045 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 2 Apr 2021 16:53:57 -0400 Subject: [PATCH 02/16] Use admin credentials with reset API --- .../org/elasticsearch/client/FeaturesIT.java | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java index e8c3463762992..8bdfdaca49fbd 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java @@ -12,9 +12,15 @@ import org.elasticsearch.client.feature.GetFeaturesResponse; import org.elasticsearch.client.feature.ResetFeaturesRequest; import org.elasticsearch.client.feature.ResetFeaturesResponse; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.search.SearchModule; import java.io.IOException; +import java.util.Collections; +import java.util.Set; +import java.util.stream.Collectors; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.notNullValue; @@ -34,13 +40,25 @@ public void testGetFeatures() throws IOException { public void testResetFeatures() throws IOException { ResetFeaturesRequest request = new ResetFeaturesRequest(); + // need superuser privileges to execute the reset + RestHighLevelClient adminHighLevelClient = new RestHighLevelClient( + adminClient(), + (client) -> {}, + new SearchModule(Settings.EMPTY, Collections.emptyList()).getNamedXContents()); ResetFeaturesResponse response = execute(request, - highLevelClient().features()::resetFeatures, highLevelClient().features()::resetFeaturesAsync); + adminHighLevelClient.features()::resetFeatures, + adminHighLevelClient.features()::resetFeaturesAsync); assertThat(response, notNullValue()); assertThat(response.getFeatures(), notNullValue()); assertThat(response.getFeatures().size(), greaterThan(1)); assertTrue(response.getFeatures().stream().anyMatch( feature -> "tasks".equals(feature.getFeatureName()) && "SUCCESS".equals(feature.getStatus()))); + + Set statuses = response.getFeatures().stream() + .map(ResetFeaturesResponse.ResetFeatureStateStatus::getStatus) + .collect(Collectors.toSet()); + + assertThat(statuses, contains("SUCCESS")); } } From 21fd604edc50e18be0938329e883a835e0092541 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 2 Apr 2021 21:48:01 -0400 Subject: [PATCH 03/16] Improve feature reset status reporting Previously, the ResetFeatureStateStatus object captured its status in a String, which meant that if we wanted to know if something succeeded or failed, we'd have to parse information out of the string. This isn't a good way of doing things. I've introduced a SUCCESS/FAILURE enum for status constants, and added a check for failures in the transport action. Instead of throwing the first exception that comes back from the resets, we now throw an exception that shows a complete response with all successes and failures. We still "fail fast" when we can't reset the features, but now the user won't have to iterate in order to find all the errors. The response message could probably be improved from here. I've got some todos in the code for reminders. --- .../snapshots/FeatureStateResetApiIT.java | 12 ++--- .../features/ResetFeatureStateResponse.java | 44 +++++++++++++++---- .../TransportResetFeatureStateAction.java | 10 ++++- .../elasticsearch/indices/SystemIndices.java | 6 +-- .../ResetFeatureStateResponseTests.java | 15 ++++--- .../xpack/transform/Transform.java | 2 +- 6 files changed, 63 insertions(+), 26 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index 454d86e8ff116..30b814b6b5a36 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -67,10 +67,10 @@ public void testResetSystemIndices() throws Exception { // call the reset API ResetFeatureStateResponse apiResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); assertThat(apiResponse.getItemList(), containsInAnyOrder( - new ResetFeatureStateResponse.ResetFeatureStateStatus("SystemIndexTestPlugin", "SUCCESS"), - new ResetFeatureStateResponse.ResetFeatureStateStatus("SecondSystemIndexTestPlugin", "SUCCESS"), - new ResetFeatureStateResponse.ResetFeatureStateStatus("EvilSystemIndexTestPlugin", "SUCCESS"), - new ResetFeatureStateResponse.ResetFeatureStateStatus("tasks", "SUCCESS") + ResetFeatureStateResponse.ResetFeatureStateStatus.success("SystemIndexTestPlugin"), + ResetFeatureStateResponse.ResetFeatureStateStatus.success("SecondSystemIndexTestPlugin"), + ResetFeatureStateResponse.ResetFeatureStateStatus.success("EvilSystemIndexTestPlugin"), + ResetFeatureStateResponse.ResetFeatureStateStatus.success("tasks") )); // verify that both indices are gone @@ -189,9 +189,9 @@ public void cleanUpFeature( Client client, ActionListener listener) { if (beEvil == false) { - listener.onResponse(new ResetFeatureStateResponse.ResetFeatureStateStatus(getFeatureName(), "SUCCESS")); + listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.success(getFeatureName())); } else { - listener.onFailure(new Exception("problem!")); + listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(getFeatureName(), "problem!")); } } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index 492b4f934f0d3..d244f95811452 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -92,31 +92,57 @@ public String toString() { */ public static class ResetFeatureStateStatus implements Writeable, ToXContentObject { private final String featureName; - private final String status; + private final Status status; + private final String exception; - public ResetFeatureStateStatus(String featureName, String status) { + public enum Status { + SUCCESS, + FAILURE + } + + public static ResetFeatureStateStatus success(String featureName) { + return new ResetFeatureStateStatus(featureName, Status.SUCCESS, null); + } + + public static ResetFeatureStateStatus failure(String featureName, String exceptionMessage) { + return new ResetFeatureStateStatus( + featureName, + Status.FAILURE, + exceptionMessage); + } + + private ResetFeatureStateStatus(String featureName, Status status, String exception) { this.featureName = featureName; this.status = status; + this.exception = exception; } ResetFeatureStateStatus(StreamInput in) throws IOException { this.featureName = in.readString(); - this.status = in.readString(); + this.status = Status.valueOf(in.readString()); + this.exception = in.readOptionalString(); } public String getFeatureName() { return this.featureName; } - public String getStatus() { + public Status getStatus() { return this.status; } + public String getException() { + return this.exception; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field("feature_name", this.featureName); builder.field("status", this.status); + if (this.exception != null) { + builder.field("exception", this.status); + } builder.endObject(); return builder; } @@ -124,7 +150,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(this.featureName); - out.writeString(this.status); + out.writeString(this.status.toString()); + out.writeOptionalString(exception); } @Override @@ -132,19 +159,20 @@ public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ResetFeatureStateStatus that = (ResetFeatureStateStatus) o; - return Objects.equals(featureName, that.featureName) && Objects.equals(status, that.status); + return Objects.equals(featureName, that.featureName) && status == that.status && Objects.equals(exception, that.exception); } @Override public int hashCode() { - return Objects.hash(featureName, status); + return Objects.hash(featureName, status, exception); } @Override public String toString() { return "ResetFeatureStateStatus{" + "featureName='" + featureName + '\'' + - ", status='" + status + '\'' + + ", status=" + status + + ", exception='" + exception + '\'' + '}'; } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java index 488c29f339d74..34713054e083a 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java @@ -60,7 +60,15 @@ protected void doExecute( GroupedActionListener groupedActionListener = new GroupedActionListener<>( listener.map(responses -> { assert features == responses.size(); - return new ResetFeatureStateResponse(new ArrayList<>(responses)); + boolean hasFailures = responses.stream() + .anyMatch(response -> response.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE); + ResetFeatureStateResponse response = new ResetFeatureStateResponse(new ArrayList<>(responses)); + if (hasFailures) { + // TODO[wrb]: return with a 207? + // TODO[wrb]: improve what's returned, not just error strings but stack traces too + throw new IllegalStateException("Error resetting feature states: " + response); + } + return response; }), systemIndices.getFeatures().size() ); diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index bb8d66808288f..ef0d9993c1def 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -344,7 +344,7 @@ public static void cleanUpFeature( if (allIndices.isEmpty()) { // if no actual indices match the pattern, we can stop here - listener.onResponse(new ResetFeatureStateStatus(name, "SUCCESS")); + listener.onResponse(ResetFeatureStateStatus.success(name)); return; } @@ -353,12 +353,12 @@ public static void cleanUpFeature( client.execute(DeleteIndexAction.INSTANCE, deleteIndexRequest, new ActionListener<>() { @Override public void onResponse(AcknowledgedResponse acknowledgedResponse) { - listener.onResponse(new ResetFeatureStateStatus(name, "SUCCESS")); + listener.onResponse(ResetFeatureStateStatus.success(name)); } @Override public void onFailure(Exception e) { - listener.onFailure(new Exception("Error resetting state for feature: " + name, e)); + listener.onResponse(ResetFeatureStateStatus.failure(name, e.getMessage())); } }); } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java index b326b07e20994..e1351f2bf1b6d 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java @@ -29,10 +29,12 @@ protected ResetFeatureStateResponse createTestInstance() { List resetStatuses = new ArrayList<>(); String feature1 = randomAlphaOfLengthBetween(4, 10); String feature2 = randomValueOtherThan(feature1, () -> randomAlphaOfLengthBetween(4, 10)); - resetStatuses.add(new ResetFeatureStateResponse.ResetFeatureStateStatus( - feature1, randomFrom("SUCCESS", "FAILURE"))); - resetStatuses.add(new ResetFeatureStateResponse.ResetFeatureStateStatus( - feature2, randomFrom("SUCCESS", "FAILURE"))); + resetStatuses.add(randomFrom( + ResetFeatureStateResponse.ResetFeatureStateStatus.success(feature1), + ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature1, "bad"))); + resetStatuses.add(randomFrom( + ResetFeatureStateResponse.ResetFeatureStateStatus.success(feature2), + ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature2, "bad"))); return new ResetFeatureStateResponse(resetStatuses); } @@ -46,8 +48,7 @@ protected ResetFeatureStateResponse mutateInstance(ResetFeatureStateResponse ins .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName) .collect(Collectors.toSet()); return new ResetFeatureStateResponse(randomList(minSize, 10, - () -> new ResetFeatureStateResponse.ResetFeatureStateStatus( - randomValueOtherThanMany(existingFeatureNames::contains, () -> randomAlphaOfLengthBetween(4, 10)), - randomAlphaOfLengthBetween(5, 10)))); + () -> ResetFeatureStateResponse.ResetFeatureStateStatus.success( + randomValueOtherThanMany(existingFeatureNames::contains, () -> randomAlphaOfLengthBetween(4, 10))))); } } diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java index 2f1ce2cf17921..fc859bdbd9b16 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java @@ -359,7 +359,7 @@ public void cleanUpFeature( + (stopTransformsResponse.getTaskFailures().isEmpty() ? "" : "task failures: " + stopTransformsResponse.getTaskFailures()); - listener.onResponse(new ResetFeatureStateResponse.ResetFeatureStateStatus(this.getFeatureName(), errMsg)); + listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(this.getFeatureName(), errMsg)); } }, listener::onFailure); From 31232252fc7d4711f9247fce85227d39a6bacf1b Mon Sep 17 00:00:00 2001 From: William Brafford Date: Wed, 21 Apr 2021 13:39:19 -0400 Subject: [PATCH 04/16] Return a 207 if some but not all reset actions fail --- .../snapshots/FeatureStateResetApiIT.java | 6 +++--- .../features/ResetFeatureStateResponse.java | 8 ++++++++ .../TransportResetFeatureStateAction.java | 10 +--------- .../cluster/RestResetFeatureStateAction.java | 17 ++++++++++++++++- 4 files changed, 28 insertions(+), 13 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index 30b814b6b5a36..f5136f271d58a 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -105,10 +105,10 @@ public void testResetSystemIndices() throws Exception { public void testFailFastOnStateCleanupFailure() throws Exception { try { EvilSystemIndexTestPlugin.beEvil = true; - Exception e1 = expectThrows(Exception.class, - () -> client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get()); + ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE, + new ResetFeatureStateRequest()).get(); - assertThat(e1.getMessage(), containsString("problem!")); + assertTrue(resetFeatureStateResponse.hasSomeFailures()); } finally { EvilSystemIndexTestPlugin.beEvil = false; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index d244f95811452..d2c89ef4cae42 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -47,6 +47,14 @@ public List getItemList() { return this.resetFeatureStateStatusList; } + public boolean hasSomeFailures() { + return resetFeatureStateStatusList.stream().anyMatch(status -> status.getStatus().equals(ResetFeatureStateStatus.Status.FAILURE)); + } + + public boolean hasAllFailures() { + return resetFeatureStateStatusList.stream().allMatch(status -> status.getStatus().equals(ResetFeatureStateStatus.Status.FAILURE)); + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java index 34713054e083a..488c29f339d74 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/TransportResetFeatureStateAction.java @@ -60,15 +60,7 @@ protected void doExecute( GroupedActionListener groupedActionListener = new GroupedActionListener<>( listener.map(responses -> { assert features == responses.size(); - boolean hasFailures = responses.stream() - .anyMatch(response -> response.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE); - ResetFeatureStateResponse response = new ResetFeatureStateResponse(new ArrayList<>(responses)); - if (hasFailures) { - // TODO[wrb]: return with a 207? - // TODO[wrb]: improve what's returned, not just error strings but stack traces too - throw new IllegalStateException("Error resetting feature states: " + response); - } - return response; + return new ResetFeatureStateResponse(new ArrayList<>(responses)); }), systemIndices.getFeatures().size() ); diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java index d3fedbc372760..a596de4f44f65 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java @@ -10,9 +10,11 @@ import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest; +import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestToXContentListener; import java.io.IOException; @@ -39,6 +41,19 @@ public String getName() { protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { final ResetFeatureStateRequest req = new ResetFeatureStateRequest(); - return restChannel -> client.execute(ResetFeatureStateAction.INSTANCE, req, new RestToXContentListener<>(restChannel)); + return restChannel -> client.execute( + ResetFeatureStateAction.INSTANCE, + req, + new RestToXContentListener<>(restChannel) { + @Override + protected RestStatus getStatus(ResetFeatureStateResponse response) { + if (response.hasAllFailures()) { + return RestStatus.INTERNAL_SERVER_ERROR; + } else if (response.hasSomeFailures()) { + return RestStatus.MULTI_STATUS; + } + return RestStatus.OK; + } + }); } } From 6e30065ec9c4c299b210b98bb0074b9a595d863e Mon Sep 17 00:00:00 2001 From: William Brafford Date: Wed, 21 Apr 2021 16:25:37 -0400 Subject: [PATCH 05/16] Serialize stack trace in usable format --- .../features/ResetFeatureStateResponse.java | 52 +++++++++++++------ .../elasticsearch/indices/SystemIndices.java | 2 +- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index d2c89ef4cae42..bb100ce30f94e 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -8,6 +8,7 @@ package org.elasticsearch.action.admin.cluster.snapshots.features; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionResponse; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -101,7 +102,7 @@ public String toString() { public static class ResetFeatureStateStatus implements Writeable, ToXContentObject { private final String featureName; private final Status status; - private final String exception; + private final Exception exception; public enum Status { SUCCESS, @@ -112,14 +113,21 @@ public static ResetFeatureStateStatus success(String featureName) { return new ResetFeatureStateStatus(featureName, Status.SUCCESS, null); } + public static ResetFeatureStateStatus failure(String featureName, Exception exception) { + return new ResetFeatureStateStatus( + featureName, + Status.FAILURE, + exception); + } + public static ResetFeatureStateStatus failure(String featureName, String exceptionMessage) { return new ResetFeatureStateStatus( featureName, Status.FAILURE, - exceptionMessage); + new ElasticsearchException(exceptionMessage)); } - private ResetFeatureStateStatus(String featureName, Status status, String exception) { + private ResetFeatureStateStatus(String featureName, Status status, Exception exception) { this.featureName = featureName; this.status = status; this.exception = exception; @@ -128,7 +136,7 @@ private ResetFeatureStateStatus(String featureName, Status status, String except ResetFeatureStateStatus(StreamInput in) throws IOException { this.featureName = in.readString(); this.status = Status.valueOf(in.readString()); - this.exception = in.readOptionalString(); + this.exception = in.readBoolean() ? in.readException() : null; } public String getFeatureName() { @@ -139,7 +147,7 @@ public Status getStatus() { return this.status; } - public String getException() { + public Exception getException() { return this.exception; } @@ -149,30 +157,44 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("feature_name", this.featureName); builder.field("status", this.status); if (this.exception != null) { - builder.field("exception", this.status); + builder.field("exception", this.exception); } builder.endObject(); return builder; } - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeString(this.featureName); - out.writeString(this.status.toString()); - out.writeOptionalString(exception); - } - + /** + * Without a convenient way to compare Exception equality, we consider + * only feature name and success or failure for equality. + * @param o An object to compare for equality + * @return True if the feature name and status are equal, false otherwise + */ @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ResetFeatureStateStatus that = (ResetFeatureStateStatus) o; - return Objects.equals(featureName, that.featureName) && status == that.status && Objects.equals(exception, that.exception); + return Objects.equals(featureName, that.featureName) && status == that.status; } + /** + * @return Hash code based only on feature name and status. + */ @Override public int hashCode() { - return Objects.hash(featureName, status, exception); + return Objects.hash(featureName, status); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(this.featureName); + out.writeString(this.status.toString()); + if (exception != null) { + out.writeBoolean(true); + out.writeException(exception); + } else { + out.writeBoolean(false); + } } @Override diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index 4d486c6fead55..e243de1593618 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -610,7 +610,7 @@ public void onResponse(AcknowledgedResponse acknowledgedResponse) { @Override public void onFailure(Exception e) { - listener.onResponse(ResetFeatureStateStatus.failure(name, e.getMessage())); + listener.onResponse(ResetFeatureStateStatus.failure(name, e)); } }); } From b039ca91ea99c2cb212f776c3d88bf0b4f148902 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Thu, 22 Apr 2021 11:59:52 -0400 Subject: [PATCH 06/16] Reduce number of iterations over response set --- .../snapshots/FeatureStateResetApiIT.java | 8 +++++++- .../snapshots/features/ResetFeatureStateResponse.java | 8 -------- .../admin/cluster/RestResetFeatureStateAction.java | 11 +++++++---- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index f5136f271d58a..f1c4f209b93cd 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -26,8 +26,10 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.stream.Collectors; import static org.hamcrest.Matchers.arrayContaining; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; @@ -108,7 +110,11 @@ public void testFailFastOnStateCleanupFailure() throws Exception { ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); - assertTrue(resetFeatureStateResponse.hasSomeFailures()); + List failedFeatures = resetFeatureStateResponse.getItemList().stream() + .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) + .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName) + .collect(Collectors.toList()); + assertThat(failedFeatures, contains("EvilSystemIndexTestPlugin")); } finally { EvilSystemIndexTestPlugin.beEvil = false; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index bb100ce30f94e..e2629ba1314d7 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -48,14 +48,6 @@ public List getItemList() { return this.resetFeatureStateStatusList; } - public boolean hasSomeFailures() { - return resetFeatureStateStatusList.stream().anyMatch(status -> status.getStatus().equals(ResetFeatureStateStatus.Status.FAILURE)); - } - - public boolean hasAllFailures() { - return resetFeatureStateStatusList.stream().allMatch(status -> status.getStatus().equals(ResetFeatureStateStatus.Status.FAILURE)); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java index a596de4f44f65..f6e5ba8d5b228 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java @@ -47,12 +47,15 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli new RestToXContentListener<>(restChannel) { @Override protected RestStatus getStatus(ResetFeatureStateResponse response) { - if (response.hasAllFailures()) { + long failures = response.getItemList().stream() + .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) + .count(); + if (failures == 0) { + return RestStatus.OK; + } else if (failures == response.getItemList().size()) { return RestStatus.INTERNAL_SERVER_ERROR; - } else if (response.hasSomeFailures()) { - return RestStatus.MULTI_STATUS; } - return RestStatus.OK; + return RestStatus.MULTI_STATUS; } }); } From e6113a3f2b10deced2888efd189d4fa57cfca8d0 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Thu, 22 Apr 2021 14:50:27 -0400 Subject: [PATCH 07/16] Add optional exception to HLRC reset feature state response --- .../client/feature/ResetFeaturesResponse.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java index 72d004021d6be..3e2d38f278443 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java @@ -47,12 +47,14 @@ public static ResetFeaturesResponse parse(XContentParser parser) { public static class ResetFeatureStateStatus { private final String featureName; private final String status; + private final Exception exception; private static final ParseField FEATURE_NAME = new ParseField("feature_name"); private static final ParseField STATUS = new ParseField("status"); + private static final ParseField EXCEPTION = new ParseField("exception"); private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - "features", true, (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1]) + "features", true, (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1], (Exception) a[2]) ); static { @@ -60,11 +62,14 @@ public static class ResetFeatureStateStatus { (p, c) -> p.text(), FEATURE_NAME, ObjectParser.ValueType.STRING); PARSER.declareField(ConstructingObjectParser.constructorArg(), (p, c) -> p.text(), STATUS, ObjectParser.ValueType.STRING); + PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), + (p, c) -> p.text(), EXCEPTION, ObjectParser.ValueType.OBJECT_OR_NULL); } - ResetFeatureStateStatus(String featureName, String status) { + ResetFeatureStateStatus(String featureName, String status, Exception exception) { this.featureName = featureName; this.status = status; + this.exception = exception; } public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) { @@ -78,5 +83,9 @@ public String getFeatureName() { public String getStatus() { return status; } + + public Exception getException() { + return exception; + } } } From deffae3564d28711cfb1c38afc54e9630fe9d857 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Thu, 22 Apr 2021 17:40:25 -0400 Subject: [PATCH 08/16] Add tests for xcontent serialization --- .../client/feature/ResetFeaturesResponse.java | 64 +++++++++++++++++-- .../snapshots/ResetFeaturesResponseTests.java | 64 +++++++++++++++++++ .../features/ResetFeatureStateResponse.java | 5 +- 3 files changed, 124 insertions(+), 9 deletions(-) create mode 100644 client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java index 3e2d38f278443..e991b06b91abc 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java @@ -8,11 +8,14 @@ package org.elasticsearch.client.feature; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; +import java.io.IOException; +import java.util.ArrayList; import java.util.List; public class ResetFeaturesResponse { @@ -40,8 +43,30 @@ public List getFeatures() { return features; } - public static ResetFeaturesResponse parse(XContentParser parser) { - return PARSER.apply(parser, null); + public static ResetFeaturesResponse parse(XContentParser parser) throws IOException { + String currentFieldName = null; + List statuses = new ArrayList<>(); + for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) { + switch (token) { + case FIELD_NAME: + currentFieldName = parser.currentName(); + break; + case START_ARRAY: + if (FEATURES.getPreferredName().equals(currentFieldName)) { + for (token = parser.nextToken(); token != XContentParser.Token.END_ARRAY; token = parser.nextToken()) { + if (token == XContentParser.Token.START_OBJECT) { + statuses.add(ResetFeatureStateStatus.parse(parser, null)); + } + } + } + break; + default: + // If unknown tokens are encounter then these should be ignored, because + // this is parsing logic on the client side. + break; + } + } + return new ResetFeaturesResponse(statuses); } public static class ResetFeatureStateStatus { @@ -51,7 +76,7 @@ public static class ResetFeatureStateStatus { private static final ParseField FEATURE_NAME = new ParseField("feature_name"); private static final ParseField STATUS = new ParseField("status"); - private static final ParseField EXCEPTION = new ParseField("exception"); + private static final ParseField ERROR = new ParseField("error"); private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( "features", true, (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1], (Exception) a[2]) @@ -63,7 +88,7 @@ public static class ResetFeatureStateStatus { PARSER.declareField(ConstructingObjectParser.constructorArg(), (p, c) -> p.text(), STATUS, ObjectParser.ValueType.STRING); PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), - (p, c) -> p.text(), EXCEPTION, ObjectParser.ValueType.OBJECT_OR_NULL); + (p, c) -> p.text(), ERROR, ObjectParser.ValueType.OBJECT_OR_NULL); } ResetFeatureStateStatus(String featureName, String status, Exception exception) { @@ -72,8 +97,35 @@ public static class ResetFeatureStateStatus { this.exception = exception; } - public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) { - return PARSER.apply(parser, ctx); + public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) throws IOException { + String currentFieldName = null; + String featureName = null; + String status = null; + Exception exception = null; + for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) { + switch (token) { + case FIELD_NAME: + currentFieldName = parser.currentName(); + break; + case VALUE_STRING: + if (FEATURE_NAME.match(currentFieldName, parser.getDeprecationHandler())) { + featureName = parser.text(); + } else if (STATUS.match(currentFieldName, parser.getDeprecationHandler())) { + status = parser.text(); + } + break; + case START_OBJECT: + if (ERROR.match(currentFieldName, parser.getDeprecationHandler())) { + exception = ElasticsearchException.fromXContent(parser); + } + break; + default: + // If unknown tokens are encounter then these should be ignored, because + // this is parsing logic on the client side. + break; + } + } + return new ResetFeatureStateStatus(featureName, status, exception); } public String getFeatureName() { diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java new file mode 100644 index 0000000000000..10d0f73c3132c --- /dev/null +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java @@ -0,0 +1,64 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.client.snapshots; + +import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; +import org.elasticsearch.client.AbstractResponseTestCase; +import org.elasticsearch.client.feature.ResetFeaturesResponse; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; + +import java.io.IOException; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.hamcrest.Matchers.everyItem; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.in; +import static org.hamcrest.Matchers.is; + +public class ResetFeaturesResponseTests extends AbstractResponseTestCase { + + @Override + protected ResetFeatureStateResponse createServerTestInstance( + XContentType xContentType) { + return new org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse( + randomList( + 10, + () -> randomBoolean() + ? ResetFeatureStateResponse.ResetFeatureStateStatus.success(randomAlphaOfLengthBetween(6, 10)) + : ResetFeatureStateResponse.ResetFeatureStateStatus.failure( + randomAlphaOfLengthBetween(6, 10), "something went wrong") + ) + ); + } + + @Override + protected ResetFeaturesResponse doParseToClientInstance(XContentParser parser) throws IOException { + return ResetFeaturesResponse.parse(parser); + } + + @Override + protected void assertInstances(ResetFeatureStateResponse serverTestInstance, ResetFeaturesResponse clientInstance) { + + assertNotNull(serverTestInstance.getItemList()); + assertNotNull(clientInstance.getFeatures()); + + assertThat(clientInstance.getFeatures(), hasSize(serverTestInstance.getItemList().size())); + + Map clientFeatures = clientInstance.getFeatures() + .stream() + .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus())); + Map serverFeatures = serverTestInstance.getItemList() + .stream() + .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus().toString())); + + assertThat(clientFeatures.entrySet(), everyItem(is(in(serverFeatures.entrySet())))); + } +} diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index e2629ba1314d7..7167937739c1d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -44,6 +44,7 @@ public ResetFeatureStateResponse(StreamInput in) throws IOException { this.resetFeatureStateStatusList = in.readList(ResetFeatureStateStatus::new); } + // TODO: rename public List getItemList() { return this.resetFeatureStateStatusList; } @@ -148,9 +149,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(); builder.field("feature_name", this.featureName); builder.field("status", this.status); - if (this.exception != null) { - builder.field("exception", this.exception); - } + ElasticsearchException.generateFailureXContent(builder, params, exception, true); builder.endObject(); return builder; } From f3f29f098fa7762a088812201d6024e95c132ae1 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Thu, 22 Apr 2021 17:41:27 -0400 Subject: [PATCH 09/16] Make method name more specific --- .../client/snapshots/ResetFeaturesResponseTests.java | 6 +++--- .../org/elasticsearch/snapshots/FeatureStateResetApiIT.java | 4 ++-- .../snapshots/features/ResetFeatureStateResponse.java | 3 +-- .../action/admin/cluster/RestResetFeatureStateAction.java | 4 ++-- .../snapshots/features/ResetFeatureStateResponseTests.java | 4 ++-- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java index 10d0f73c3132c..77023bccc9163 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java @@ -47,15 +47,15 @@ protected ResetFeaturesResponse doParseToClientInstance(XContentParser parser) t @Override protected void assertInstances(ResetFeatureStateResponse serverTestInstance, ResetFeaturesResponse clientInstance) { - assertNotNull(serverTestInstance.getItemList()); + assertNotNull(serverTestInstance.getFeatureStateResetStatusList()); assertNotNull(clientInstance.getFeatures()); - assertThat(clientInstance.getFeatures(), hasSize(serverTestInstance.getItemList().size())); + assertThat(clientInstance.getFeatures(), hasSize(serverTestInstance.getFeatureStateResetStatusList().size())); Map clientFeatures = clientInstance.getFeatures() .stream() .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus())); - Map serverFeatures = serverTestInstance.getItemList() + Map serverFeatures = serverTestInstance.getFeatureStateResetStatusList() .stream() .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus().toString())); diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index f1c4f209b93cd..c3c6527f6a398 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -68,7 +68,7 @@ public void testResetSystemIndices() throws Exception { // call the reset API ResetFeatureStateResponse apiResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); - assertThat(apiResponse.getItemList(), containsInAnyOrder( + assertThat(apiResponse.getFeatureStateResetStatusList(), containsInAnyOrder( ResetFeatureStateResponse.ResetFeatureStateStatus.success("SystemIndexTestPlugin"), ResetFeatureStateResponse.ResetFeatureStateStatus.success("SecondSystemIndexTestPlugin"), ResetFeatureStateResponse.ResetFeatureStateStatus.success("EvilSystemIndexTestPlugin"), @@ -110,7 +110,7 @@ public void testFailFastOnStateCleanupFailure() throws Exception { ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); - List failedFeatures = resetFeatureStateResponse.getItemList().stream() + List failedFeatures = resetFeatureStateResponse.getFeatureStateResetStatusList().stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName) .collect(Collectors.toList()); diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index 7167937739c1d..3a8ca31d69ffb 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -44,8 +44,7 @@ public ResetFeatureStateResponse(StreamInput in) throws IOException { this.resetFeatureStateStatusList = in.readList(ResetFeatureStateStatus::new); } - // TODO: rename - public List getItemList() { + public List getFeatureStateResetStatusList() { return this.resetFeatureStateStatusList; } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java index f6e5ba8d5b228..26f387db532e7 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java @@ -47,12 +47,12 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli new RestToXContentListener<>(restChannel) { @Override protected RestStatus getStatus(ResetFeatureStateResponse response) { - long failures = response.getItemList().stream() + long failures = response.getFeatureStateResetStatusList().stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .count(); if (failures == 0) { return RestStatus.OK; - } else if (failures == response.getItemList().size()) { + } else if (failures == response.getFeatureStateResetStatusList().size()) { return RestStatus.INTERNAL_SERVER_ERROR; } return RestStatus.MULTI_STATUS; diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java index e1351f2bf1b6d..85028b6156861 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java @@ -41,10 +41,10 @@ protected ResetFeatureStateResponse createTestInstance() { @Override protected ResetFeatureStateResponse mutateInstance(ResetFeatureStateResponse instance) throws IOException { int minSize = 0; - if (instance.getItemList().size() == 0) { + if (instance.getFeatureStateResetStatusList().size() == 0) { minSize = 1; } - Set existingFeatureNames = instance.getItemList().stream() + Set existingFeatureNames = instance.getFeatureStateResetStatusList().stream() .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName) .collect(Collectors.toSet()); return new ResetFeatureStateResponse(randomList(minSize, 10, From d2c1b459f011dd0e2d2f6decae566d8132bcdced Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 23 Apr 2021 10:32:21 -0400 Subject: [PATCH 10/16] Add a line to the docs about response codes --- docs/reference/features/apis/reset-features-api.asciidoc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/reference/features/apis/reset-features-api.asciidoc b/docs/reference/features/apis/reset-features-api.asciidoc index 2d17825a39d96..300ce166eaa13 100644 --- a/docs/reference/features/apis/reset-features-api.asciidoc +++ b/docs/reference/features/apis/reset-features-api.asciidoc @@ -8,7 +8,7 @@ experimental::[] Clears all of the the state information stored in system indices by {es} features, including the security and machine learning indices. -WARNING: Intended for development and testing use only. Do not reset features on a production cluster. +WARNING: Intended for development and testing use only. Do not reset features on a production cluster. [source,console] ----------------------------------- @@ -26,9 +26,11 @@ POST /_features/_reset Return a cluster to the same state as a new installation by resetting the feature state for all {es} features. This deletes all state information stored in system indices. -Note that select features might provide a way to reset particular system indices. Using this API resets _all_ features, both those that are built-in and implemented as plugins. +The response code is `HTTP 200` if state is successfully reset for all features, `HTTP 207` if there is a mixture of successes and failures, and `HTTP 500` if the reset operation fails for all features. -To list the features that will be affected, use the <>. +Note that select features might provide a way to reset particular system indices. Using this API resets _all_ features, both those that are built-in and implemented as plugins. + +To list the features that will be affected, use the <>. IMPORTANT: The features installed on the node you submit this request to are the features that will be reset. Run on the master node if you have any doubts about which plugins are installed on individual nodes. From cd047f2b9d34bf513c26aac6656720d2a49da71e Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 23 Apr 2021 15:08:44 -0400 Subject: [PATCH 11/16] Use constructing object parser to handle exception --- .../client/feature/ResetFeaturesResponse.java | 70 +++---------------- 1 file changed, 10 insertions(+), 60 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java index e991b06b91abc..aa0dc38d94e8f 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java @@ -14,8 +14,6 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; -import java.io.IOException; -import java.util.ArrayList; import java.util.List; public class ResetFeaturesResponse { @@ -43,30 +41,8 @@ public List getFeatures() { return features; } - public static ResetFeaturesResponse parse(XContentParser parser) throws IOException { - String currentFieldName = null; - List statuses = new ArrayList<>(); - for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) { - switch (token) { - case FIELD_NAME: - currentFieldName = parser.currentName(); - break; - case START_ARRAY: - if (FEATURES.getPreferredName().equals(currentFieldName)) { - for (token = parser.nextToken(); token != XContentParser.Token.END_ARRAY; token = parser.nextToken()) { - if (token == XContentParser.Token.START_OBJECT) { - statuses.add(ResetFeatureStateStatus.parse(parser, null)); - } - } - } - break; - default: - // If unknown tokens are encounter then these should be ignored, because - // this is parsing logic on the client side. - break; - } - } - return new ResetFeaturesResponse(statuses); + public static ResetFeaturesResponse parse(XContentParser parser) { + return PARSER.apply(parser, null); } public static class ResetFeatureStateStatus { @@ -76,10 +52,11 @@ public static class ResetFeatureStateStatus { private static final ParseField FEATURE_NAME = new ParseField("feature_name"); private static final ParseField STATUS = new ParseField("status"); - private static final ParseField ERROR = new ParseField("error"); + private static final ParseField EXCEPTION = new ParseField("exception"); - private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - "features", true, (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1], (Exception) a[2]) + private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + "features", true, + (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1], (ElasticsearchException) a[2]) ); static { @@ -87,8 +64,8 @@ public static class ResetFeatureStateStatus { (p, c) -> p.text(), FEATURE_NAME, ObjectParser.ValueType.STRING); PARSER.declareField(ConstructingObjectParser.constructorArg(), (p, c) -> p.text(), STATUS, ObjectParser.ValueType.STRING); - PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), - (p, c) -> p.text(), ERROR, ObjectParser.ValueType.OBJECT_OR_NULL); + PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), + (p, c) -> ElasticsearchException.failureFromXContent(p), EXCEPTION); } ResetFeatureStateStatus(String featureName, String status, Exception exception) { @@ -97,35 +74,8 @@ public static class ResetFeatureStateStatus { this.exception = exception; } - public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) throws IOException { - String currentFieldName = null; - String featureName = null; - String status = null; - Exception exception = null; - for (XContentParser.Token token = parser.nextToken(); token != XContentParser.Token.END_OBJECT; token = parser.nextToken()) { - switch (token) { - case FIELD_NAME: - currentFieldName = parser.currentName(); - break; - case VALUE_STRING: - if (FEATURE_NAME.match(currentFieldName, parser.getDeprecationHandler())) { - featureName = parser.text(); - } else if (STATUS.match(currentFieldName, parser.getDeprecationHandler())) { - status = parser.text(); - } - break; - case START_OBJECT: - if (ERROR.match(currentFieldName, parser.getDeprecationHandler())) { - exception = ElasticsearchException.fromXContent(parser); - } - break; - default: - // If unknown tokens are encounter then these should be ignored, because - // this is parsing logic on the client side. - break; - } - } - return new ResetFeatureStateStatus(featureName, status, exception); + public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) { + return PARSER.apply(parser, ctx); } public String getFeatureName() { From 83f20191387e5a0857002b42a0e7645182c4defe Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 26 Apr 2021 15:12:42 -0400 Subject: [PATCH 12/16] Improve naming in ResetFeaturesResponse and add javadoc --- .../client/feature/ResetFeaturesResponse.java | 44 +++++++++++++++++-- .../org/elasticsearch/client/FeaturesIT.java | 8 ++-- .../snapshots/ResetFeaturesResponseTests.java | 6 +-- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java index aa0dc38d94e8f..60ff9227c0c4b 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java @@ -9,13 +9,19 @@ package org.elasticsearch.client.feature; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; import java.util.List; +import java.util.Objects; +/** + * This class represents the response of the Feature State Reset API. It is a + * list containing the response of every feature whose state was reset. + */ public class ResetFeaturesResponse { private final List features; @@ -23,7 +29,7 @@ public class ResetFeaturesResponse { @SuppressWarnings("unchecked") private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - "snapshottable_features_response", true, + "features_reset_status_response", true, (a, ctx) -> new ResetFeaturesResponse((List) a[0]) ); @@ -33,11 +39,18 @@ public class ResetFeaturesResponse { ResetFeaturesResponse.ResetFeatureStateStatus::parse, FEATURES); } + /** + * Create a new ResetFeaturesResponse + * @param features A full list of status responses from individual feature reset operations. + */ public ResetFeaturesResponse(List features) { this.features = features; } - public List getFeatures() { + /** + * @return List containing a reset status for each feature that we have tried to reset. + */ + public List getFeatureResetStatuses() { return features; } @@ -45,6 +58,12 @@ public static ResetFeaturesResponse parse(XContentParser parser) { return PARSER.apply(parser, null); } + /** + * A class representing the status of an attempt to reset a feature's state. + * The attempt to reset either succeeds and we return the name of the + * feature and a success flag; or it fails and we return the name of the feature, + * a status flag, and the exception thrown during the attempt to reset the feature. + */ public static class ResetFeatureStateStatus { private final String featureName; private final String status; @@ -68,9 +87,18 @@ public static class ResetFeatureStateStatus { (p, c) -> ElasticsearchException.failureFromXContent(p), EXCEPTION); } - ResetFeatureStateStatus(String featureName, String status, Exception exception) { + /** + * Create a ResetFeatureStateStatus. + * @param featureName Name of the feature whose status has been reset. + * @param status Whether the reset attempt succeeded or failed. + * @param exception If the reset attempt failed, the exception that caused the + * failure. Must be null when status is "SUCCESS". + */ + ResetFeatureStateStatus(String featureName, String status, @Nullable Exception exception) { this.featureName = featureName; + assert "SUCCESS".equals(status) || "FAILURE".equals(status); this.status = status; + assert "FAILURE".equals(status) ? Objects.nonNull(exception) : Objects.isNull(exception); this.exception = exception; } @@ -78,14 +106,24 @@ public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) { return PARSER.apply(parser, ctx); } + /** + * @return Name of the feature that we tried to reset + */ public String getFeatureName() { return featureName; } + /** + * @return "SUCCESS" if the reset attempt succeeded, "FAILURE" otherwise. + */ public String getStatus() { return status; } + /** + * @return The exception that caused the reset attempt to fail. + */ + @Nullable public Exception getException() { return exception; } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java index 8bdfdaca49fbd..99526f0ec4c56 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java @@ -50,12 +50,12 @@ public void testResetFeatures() throws IOException { adminHighLevelClient.features()::resetFeaturesAsync); assertThat(response, notNullValue()); - assertThat(response.getFeatures(), notNullValue()); - assertThat(response.getFeatures().size(), greaterThan(1)); - assertTrue(response.getFeatures().stream().anyMatch( + assertThat(response.getFeatureResetStatuses(), notNullValue()); + assertThat(response.getFeatureResetStatuses().size(), greaterThan(1)); + assertTrue(response.getFeatureResetStatuses().stream().anyMatch( feature -> "tasks".equals(feature.getFeatureName()) && "SUCCESS".equals(feature.getStatus()))); - Set statuses = response.getFeatures().stream() + Set statuses = response.getFeatureResetStatuses().stream() .map(ResetFeaturesResponse.ResetFeatureStateStatus::getStatus) .collect(Collectors.toSet()); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java index 77023bccc9163..f4536d94ce5fd 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java @@ -48,11 +48,11 @@ protected ResetFeaturesResponse doParseToClientInstance(XContentParser parser) t protected void assertInstances(ResetFeatureStateResponse serverTestInstance, ResetFeaturesResponse clientInstance) { assertNotNull(serverTestInstance.getFeatureStateResetStatusList()); - assertNotNull(clientInstance.getFeatures()); + assertNotNull(clientInstance.getFeatureResetStatuses()); - assertThat(clientInstance.getFeatures(), hasSize(serverTestInstance.getFeatureStateResetStatusList().size())); + assertThat(clientInstance.getFeatureResetStatuses(), hasSize(serverTestInstance.getFeatureStateResetStatusList().size())); - Map clientFeatures = clientInstance.getFeatures() + Map clientFeatures = clientInstance.getFeatureResetStatuses() .stream() .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus())); Map serverFeatures = serverTestInstance.getFeatureStateResetStatusList() From 19e4842ffd39a7bdc6825d25c1114d18fcf85443 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 26 Apr 2021 15:13:34 -0400 Subject: [PATCH 13/16] Use concurrency-safe accessors for static variable --- .../snapshots/FeatureStateResetApiIT.java | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index c3c6527f6a398..8e1bd18efdf3c 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -32,6 +32,7 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.notNullValue; public class FeatureStateResetApiIT extends ESIntegTestCase { @@ -102,21 +103,27 @@ public void testResetSystemIndices() throws Exception { } /** - * Evil test - what if the cleanup method fails? + * Evil test - test that when a feature fails to reset, we get a response object + * indicating the failure */ - public void testFailFastOnStateCleanupFailure() throws Exception { + public void testFeatureResetFailure() throws Exception { try { - EvilSystemIndexTestPlugin.beEvil = true; + EvilSystemIndexTestPlugin.setBeEvil(true); ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); List failedFeatures = resetFeatureStateResponse.getFeatureStateResetStatusList().stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) - .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName) + .peek(status -> assertThat(status.getException(), notNullValue())) + .map(status -> { + // all failed statuses should have exceptions + assertThat(status.getException(), notNullValue()); + return status.getFeatureName(); + }) .collect(Collectors.toList()); assertThat(failedFeatures, contains("EvilSystemIndexTestPlugin")); } finally { - EvilSystemIndexTestPlugin.beEvil = false; + EvilSystemIndexTestPlugin.setBeEvil(false); } } @@ -189,15 +196,23 @@ public String getFeatureDescription() { return "a plugin that can be very bad"; } + public static synchronized void setBeEvil(boolean evil) { + beEvil = evil; + } + + public static synchronized boolean isEvil() { + return beEvil; + } + @Override public void cleanUpFeature( ClusterService clusterService, Client client, ActionListener listener) { - if (beEvil == false) { - listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.success(getFeatureName())); - } else { + if (isEvil()) { listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(getFeatureName(), "problem!")); + } else { + listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.success(getFeatureName())); } } } From d277a24e885f738725d7ccd69460ef290f6c6d8c Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 26 Apr 2021 16:06:40 -0400 Subject: [PATCH 14/16] Respond to PR feedback --- .../snapshots/ResetFeaturesResponseTests.java | 9 ++-- .../snapshots/FeatureStateResetApiIT.java | 8 +-- .../features/ResetFeatureStateResponse.java | 50 ++++++++++++++----- .../cluster/RestResetFeatureStateAction.java | 4 +- .../ResetFeatureStateResponseTests.java | 9 ++-- .../xpack/transform/Transform.java | 3 +- 6 files changed, 56 insertions(+), 27 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java index f4536d94ce5fd..d21f2a469f33d 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.client.snapshots; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; import org.elasticsearch.client.AbstractResponseTestCase; import org.elasticsearch.client.feature.ResetFeaturesResponse; @@ -34,7 +35,7 @@ protected ResetFeatureStateResponse createServerTestInstance( () -> randomBoolean() ? ResetFeatureStateResponse.ResetFeatureStateStatus.success(randomAlphaOfLengthBetween(6, 10)) : ResetFeatureStateResponse.ResetFeatureStateStatus.failure( - randomAlphaOfLengthBetween(6, 10), "something went wrong") + randomAlphaOfLengthBetween(6, 10), new ElasticsearchException("something went wrong")) ) ); } @@ -47,15 +48,15 @@ protected ResetFeaturesResponse doParseToClientInstance(XContentParser parser) t @Override protected void assertInstances(ResetFeatureStateResponse serverTestInstance, ResetFeaturesResponse clientInstance) { - assertNotNull(serverTestInstance.getFeatureStateResetStatusList()); + assertNotNull(serverTestInstance.getFeatureStateResetStatuses()); assertNotNull(clientInstance.getFeatureResetStatuses()); - assertThat(clientInstance.getFeatureResetStatuses(), hasSize(serverTestInstance.getFeatureStateResetStatusList().size())); + assertThat(clientInstance.getFeatureResetStatuses(), hasSize(serverTestInstance.getFeatureStateResetStatuses().size())); Map clientFeatures = clientInstance.getFeatureResetStatuses() .stream() .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus())); - Map serverFeatures = serverTestInstance.getFeatureStateResetStatusList() + Map serverFeatures = serverTestInstance.getFeatureStateResetStatuses() .stream() .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus().toString())); diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java index 8e1bd18efdf3c..1791760daee3e 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java @@ -8,6 +8,7 @@ package org.elasticsearch.snapshots; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest; @@ -69,7 +70,7 @@ public void testResetSystemIndices() throws Exception { // call the reset API ResetFeatureStateResponse apiResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); - assertThat(apiResponse.getFeatureStateResetStatusList(), containsInAnyOrder( + assertThat(apiResponse.getFeatureStateResetStatuses(), containsInAnyOrder( ResetFeatureStateResponse.ResetFeatureStateStatus.success("SystemIndexTestPlugin"), ResetFeatureStateResponse.ResetFeatureStateStatus.success("SecondSystemIndexTestPlugin"), ResetFeatureStateResponse.ResetFeatureStateStatus.success("EvilSystemIndexTestPlugin"), @@ -112,7 +113,7 @@ public void testFeatureResetFailure() throws Exception { ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get(); - List failedFeatures = resetFeatureStateResponse.getFeatureStateResetStatusList().stream() + List failedFeatures = resetFeatureStateResponse.getFeatureStateResetStatuses().stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .peek(status -> assertThat(status.getException(), notNullValue())) .map(status -> { @@ -210,7 +211,8 @@ public void cleanUpFeature( Client client, ActionListener listener) { if (isEvil()) { - listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(getFeatureName(), "problem!")); + listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(getFeatureName(), + new ElasticsearchException("problem!"))); } else { listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.success(getFeatureName())); } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index 3a8ca31d69ffb..aae9459c6c47f 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -10,6 +10,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.common.Nullable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -18,6 +19,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Objects; @@ -31,11 +33,11 @@ public class ResetFeatureStateResponse extends ActionResponse implements ToXCont * Create a response showing which features have had state reset and success * or failure status. * - * @param statusList A list of status responses + * @param resetFeatureStateStatuses A list of status responses */ - public ResetFeatureStateResponse(List statusList) { + public ResetFeatureStateResponse(List resetFeatureStateStatuses) { resetFeatureStateStatusList = new ArrayList<>(); - resetFeatureStateStatusList.addAll(statusList); + resetFeatureStateStatusList.addAll(resetFeatureStateStatuses); resetFeatureStateStatusList.sort(Comparator.comparing(ResetFeatureStateStatus::getFeatureName)); } @@ -44,8 +46,11 @@ public ResetFeatureStateResponse(StreamInput in) throws IOException { this.resetFeatureStateStatusList = in.readList(ResetFeatureStateStatus::new); } - public List getFeatureStateResetStatusList() { - return this.resetFeatureStateStatusList; + /** + * @return List of statuses for individual reset operations, one per feature that we tried to reset + */ + public List getFeatureStateResetStatuses() { + return Collections.unmodifiableList(this.resetFeatureStateStatusList); } @Override @@ -96,15 +101,30 @@ public static class ResetFeatureStateStatus implements Writeable, ToXContentObje private final Status status; private final Exception exception; + /** + * Success or failure enum. Not a boolean so that we can easily display + * "SUCCESS" or "FAILURE" when this object is serialized. + */ public enum Status { SUCCESS, FAILURE } + /** + * Create a feature status for a successful reset operation + * @param featureName Name of the feature whose state was successfully reset + * @return Success status for a feature + */ public static ResetFeatureStateStatus success(String featureName) { return new ResetFeatureStateStatus(featureName, Status.SUCCESS, null); } + /** + * Create a feature status for a failed reset operation + * @param featureName Name of the feature that failed + * @param exception The exception that caused or described the failure + * @return Failure status for a feature + */ public static ResetFeatureStateStatus failure(String featureName, Exception exception) { return new ResetFeatureStateStatus( featureName, @@ -112,16 +132,10 @@ public static ResetFeatureStateStatus failure(String featureName, Exception exce exception); } - public static ResetFeatureStateStatus failure(String featureName, String exceptionMessage) { - return new ResetFeatureStateStatus( - featureName, - Status.FAILURE, - new ElasticsearchException(exceptionMessage)); - } - - private ResetFeatureStateStatus(String featureName, Status status, Exception exception) { + private ResetFeatureStateStatus(String featureName, Status status, @Nullable Exception exception) { this.featureName = featureName; this.status = status; + assert Status.FAILURE.equals(status) ? Objects.nonNull(exception) : Objects.isNull(exception); this.exception = exception; } @@ -131,14 +145,24 @@ private ResetFeatureStateStatus(String featureName, Status status, Exception exc this.exception = in.readBoolean() ? in.readException() : null; } + /** + * @return Name of the feature we tried to reset + */ public String getFeatureName() { return this.featureName; } + /** + * @return Success or failure for the reset operation + */ public Status getStatus() { return this.status; } + /** + * @return For a failed reset operation, the exception that caused or describes the failure. + */ + @Nullable public Exception getException() { return this.exception; } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java index 26f387db532e7..3ea40d2afbeac 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java @@ -47,12 +47,12 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli new RestToXContentListener<>(restChannel) { @Override protected RestStatus getStatus(ResetFeatureStateResponse response) { - long failures = response.getFeatureStateResetStatusList().stream() + long failures = response.getFeatureStateResetStatuses().stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .count(); if (failures == 0) { return RestStatus.OK; - } else if (failures == response.getFeatureStateResetStatusList().size()) { + } else if (failures == response.getFeatureStateResetStatuses().size()) { return RestStatus.INTERNAL_SERVER_ERROR; } return RestStatus.MULTI_STATUS; diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java index 85028b6156861..55f63f5b3aca9 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.action.admin.cluster.snapshots.features; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.test.AbstractWireSerializingTestCase; @@ -31,20 +32,20 @@ protected ResetFeatureStateResponse createTestInstance() { String feature2 = randomValueOtherThan(feature1, () -> randomAlphaOfLengthBetween(4, 10)); resetStatuses.add(randomFrom( ResetFeatureStateResponse.ResetFeatureStateStatus.success(feature1), - ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature1, "bad"))); + ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature1, new ElasticsearchException("bad")))); resetStatuses.add(randomFrom( ResetFeatureStateResponse.ResetFeatureStateStatus.success(feature2), - ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature2, "bad"))); + ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature2, new ElasticsearchException("bad")))); return new ResetFeatureStateResponse(resetStatuses); } @Override protected ResetFeatureStateResponse mutateInstance(ResetFeatureStateResponse instance) throws IOException { int minSize = 0; - if (instance.getFeatureStateResetStatusList().size() == 0) { + if (instance.getFeatureStateResetStatuses().size() == 0) { minSize = 1; } - Set existingFeatureNames = instance.getFeatureStateResetStatusList().stream() + Set existingFeatureNames = instance.getFeatureStateResetStatuses().stream() .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName) .collect(Collectors.toSet()); return new ResetFeatureStateResponse(randomList(minSize, 10, diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java index 3ce77d82dfa34..cf5774d6228c1 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java @@ -414,7 +414,8 @@ public void cleanUpFeature( + (stopTransformsResponse.getTaskFailures().isEmpty() ? "" : "task failures: " + stopTransformsResponse.getTaskFailures()); - unsetResetModeListener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(this.getFeatureName(), errMsg)); + unsetResetModeListener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(this.getFeatureName(), + new ElasticsearchException(errMsg))); } }, unsetResetModeListener::onFailure); From 9e2ab6f3e0e1256f3d37f6ee11469b4da20950da Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 26 Apr 2021 16:59:42 -0400 Subject: [PATCH 15/16] Pity me and my XContent woes --- .../client/feature/ResetFeaturesResponse.java | 4 ++-- .../snapshots/features/ResetFeatureStateResponse.java | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java index 60ff9227c0c4b..0cfe0d8bcecc2 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java @@ -74,7 +74,7 @@ public static class ResetFeatureStateStatus { private static final ParseField EXCEPTION = new ParseField("exception"); private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - "features", true, + "feature_state_reset_stats", true, (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1], (ElasticsearchException) a[2]) ); @@ -84,7 +84,7 @@ public static class ResetFeatureStateStatus { PARSER.declareField(ConstructingObjectParser.constructorArg(), (p, c) -> p.text(), STATUS, ObjectParser.ValueType.STRING); PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), - (p, c) -> ElasticsearchException.failureFromXContent(p), EXCEPTION); + (p, c) -> ElasticsearchException.fromXContent(p), EXCEPTION); } /** diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java index aae9459c6c47f..0c47c38ab726d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java @@ -172,7 +172,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(); builder.field("feature_name", this.featureName); builder.field("status", this.status); - ElasticsearchException.generateFailureXContent(builder, params, exception, true); + if (Objects.nonNull(this.exception)) { + builder.field("exception"); + builder.startObject(); + new ElasticsearchException(exception).toXContent(builder, params); + builder.endObject(); + } builder.endObject(); return builder; } From ff5a9d89d88f300d3a07f8abc2d6e0dcfc687a4c Mon Sep 17 00:00:00 2001 From: William Brafford Date: Tue, 27 Apr 2021 10:32:48 -0400 Subject: [PATCH 16/16] Improve javadoc Co-authored-by: Jay Modi --- .../elasticsearch/client/feature/ResetFeaturesResponse.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java index 0cfe0d8bcecc2..a8798842c5ffa 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java @@ -20,7 +20,9 @@ /** * This class represents the response of the Feature State Reset API. It is a - * list containing the response of every feature whose state was reset. + * list containing the response of every feature whose state can be reset. The + * response from each feature will indicate success or failure. In the case of a + * failure, the cause will be returned as well. */ public class ResetFeaturesResponse { private final List features;