From c1486012b46de80e62a77633489bc1fafeaefe6b Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Wed, 28 Jun 2017 15:16:54 -0500 Subject: [PATCH 1/2] Output all empty snapshot info fields if in verbose mode In #24477, a less verbose option was added to retrieve snapshot info via GET /_snapshot/{repo}/{snapshots}. The point of adding this less verbose option was so that if the repository is a cloud based one, and there are many snapshots for which the snapshot info needed to be retrieved, then each snapshot would require reading a separate snapshot metadata file to pull out the necessary information. This can be costly (performance and cost) on cloud based repositories, so a less verbose option was added that only retrieves very basic information about each snapshot that is all available in the index-N blob - requiring only one read! In order to display this less verbose snapshot info appropriately, logic was added to not display those fields which could not be populated. However, this broke integrators (e.g. ECE) that required these fields to be present, even if empty. This commit is to return these fields in the response, even if empty, if the verbose option is set. --- .../admin/cluster/RestGetSnapshotsAction.java | 23 ++++++++++++++++--- .../elasticsearch/snapshots/SnapshotInfo.java | 11 +++++---- .../test/snapshot.get/10_basic.yml | 3 +++ 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index f42180b5029b8..160bada6474d0 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -20,18 +20,26 @@ package org.elasticsearch.rest.action.admin.cluster; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest; +import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.rest.BaseRestHandler; +import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.action.RestToXContentListener; +import org.elasticsearch.rest.RestResponse; +import org.elasticsearch.rest.action.RestBuilderListener; import java.io.IOException; +import java.util.Collections; +import java.util.Map; import static org.elasticsearch.client.Requests.getSnapshotsRequest; import static org.elasticsearch.rest.RestRequest.Method.GET; +import static org.elasticsearch.rest.RestStatus.OK; /** * Returns information about snapshot @@ -52,10 +60,19 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC String repository = request.param("repository"); String[] snapshots = request.paramAsStringArray("snapshot", Strings.EMPTY_ARRAY); - GetSnapshotsRequest getSnapshotsRequest = getSnapshotsRequest(repository).snapshots(snapshots); + final GetSnapshotsRequest getSnapshotsRequest = getSnapshotsRequest(repository).snapshots(snapshots); getSnapshotsRequest.ignoreUnavailable(request.paramAsBoolean("ignore_unavailable", getSnapshotsRequest.ignoreUnavailable())); getSnapshotsRequest.verbose(request.paramAsBoolean("verbose", getSnapshotsRequest.verbose())); getSnapshotsRequest.masterNodeTimeout(request.paramAsTime("master_timeout", getSnapshotsRequest.masterNodeTimeout())); - return channel -> client.admin().cluster().getSnapshots(getSnapshotsRequest, new RestToXContentListener<>(channel)); + return channel -> client.admin().cluster().getSnapshots(getSnapshotsRequest, + new RestBuilderListener(channel) { + @Override + public RestResponse buildResponse(GetSnapshotsResponse response, XContentBuilder builder) throws Exception { + Map params = Collections.singletonMap("verbose", Boolean.toString(getSnapshotsRequest.verbose())); + ToXContent.MapParams xContentParams = new ToXContent.MapParams(params); + response.toXContent(builder, xContentParams); + return new BytesRestResponse(OK, builder); + } + }); } } diff --git a/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java b/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java index a54e72159f8ae..de6a4e2987e25 100644 --- a/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java +++ b/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java @@ -346,6 +346,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa return toXContentSnapshot(builder, params); } + final boolean verbose = params.paramAsBoolean("verbose", false); // write snapshot info for the API and any other situations builder.startObject(); builder.field(SNAPSHOT, snapshotId.getName()); @@ -359,22 +360,22 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa builder.value(index); } builder.endArray(); - if (state != null) { + if (verbose || state != null) { builder.field(STATE, state); } if (reason != null) { builder.field(REASON, reason); } - if (startTime != 0) { + if (verbose || startTime != 0) { builder.field(START_TIME, DATE_TIME_FORMATTER.printer().print(startTime)); builder.field(START_TIME_IN_MILLIS, startTime); } - if (endTime != 0) { + if (verbose || endTime != 0) { builder.field(END_TIME, DATE_TIME_FORMATTER.printer().print(endTime)); builder.field(END_TIME_IN_MILLIS, endTime); builder.timeValueField(DURATION_IN_MILLIS, DURATION, endTime - startTime); } - if (!shardFailures.isEmpty()) { + if (verbose || !shardFailures.isEmpty()) { builder.startArray(FAILURES); for (SnapshotShardFailure shardFailure : shardFailures) { builder.startObject(); @@ -383,7 +384,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa } builder.endArray(); } - if (totalShards != 0) { + if (verbose || totalShards != 0) { builder.startObject(SHARDS); builder.field(TOTAL, totalShards); builder.field(FAILED, failedShards()); diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get/10_basic.yml index 515662355da3e..70008415122d3 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get/10_basic.yml @@ -32,6 +32,7 @@ setup: snapshot: test_snapshot - is_true: snapshots + - is_true: snapshots.0.failures - do: snapshot.delete: @@ -87,6 +88,8 @@ setup: - is_true: snapshots - match: { snapshots.0.snapshot: test_snapshot } - match: { snapshots.0.state: SUCCESS } + - is_false: snapshots.0.failures + - is_false: snapshots.0.shards - is_false: snapshots.0.version - do: From 3d34d08dcaa1244a3aacc306025bdf3f64c3e045 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Wed, 28 Jun 2017 15:47:43 -0500 Subject: [PATCH 2/2] use RestRequest params --- .../snapshots/get/GetSnapshotsRequest.java | 3 ++- .../admin/cluster/RestGetSnapshotsAction.java | 23 +++---------------- .../elasticsearch/snapshots/SnapshotInfo.java | 3 ++- 3 files changed, 7 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java index e90f9e578ce37..32c9493b440d5 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java @@ -37,6 +37,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest public static final String ALL_SNAPSHOTS = "_all"; public static final String CURRENT_SNAPSHOT = "_current"; + public static final boolean DEFAULT_VERBOSE_MODE = true; private String repository; @@ -44,7 +45,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest private boolean ignoreUnavailable; - private boolean verbose = true; + private boolean verbose = DEFAULT_VERBOSE_MODE; public GetSnapshotsRequest() { } diff --git a/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index 160bada6474d0..f42180b5029b8 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -20,26 +20,18 @@ package org.elasticsearch.rest.action.admin.cluster; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest; -import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.rest.BaseRestHandler; -import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.rest.RestResponse; -import org.elasticsearch.rest.action.RestBuilderListener; +import org.elasticsearch.rest.action.RestToXContentListener; import java.io.IOException; -import java.util.Collections; -import java.util.Map; import static org.elasticsearch.client.Requests.getSnapshotsRequest; import static org.elasticsearch.rest.RestRequest.Method.GET; -import static org.elasticsearch.rest.RestStatus.OK; /** * Returns information about snapshot @@ -60,19 +52,10 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC String repository = request.param("repository"); String[] snapshots = request.paramAsStringArray("snapshot", Strings.EMPTY_ARRAY); - final GetSnapshotsRequest getSnapshotsRequest = getSnapshotsRequest(repository).snapshots(snapshots); + GetSnapshotsRequest getSnapshotsRequest = getSnapshotsRequest(repository).snapshots(snapshots); getSnapshotsRequest.ignoreUnavailable(request.paramAsBoolean("ignore_unavailable", getSnapshotsRequest.ignoreUnavailable())); getSnapshotsRequest.verbose(request.paramAsBoolean("verbose", getSnapshotsRequest.verbose())); getSnapshotsRequest.masterNodeTimeout(request.paramAsTime("master_timeout", getSnapshotsRequest.masterNodeTimeout())); - return channel -> client.admin().cluster().getSnapshots(getSnapshotsRequest, - new RestBuilderListener(channel) { - @Override - public RestResponse buildResponse(GetSnapshotsResponse response, XContentBuilder builder) throws Exception { - Map params = Collections.singletonMap("verbose", Boolean.toString(getSnapshotsRequest.verbose())); - ToXContent.MapParams xContentParams = new ToXContent.MapParams(params); - response.toXContent(builder, xContentParams); - return new BytesRestResponse(OK, builder); - } - }); + return channel -> client.admin().cluster().getSnapshots(getSnapshotsRequest, new RestToXContentListener<>(channel)); } } diff --git a/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java b/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java index de6a4e2987e25..de0a52ed0e46a 100644 --- a/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java +++ b/core/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java @@ -21,6 +21,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.action.ShardOperationFailedException; +import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -346,7 +347,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa return toXContentSnapshot(builder, params); } - final boolean verbose = params.paramAsBoolean("verbose", false); + final boolean verbose = params.paramAsBoolean("verbose", GetSnapshotsRequest.DEFAULT_VERBOSE_MODE); // write snapshot info for the API and any other situations builder.startObject(); builder.field(SNAPSHOT, snapshotId.getName());