Clean Up Snapshot Create Rest API #31779

jdconrad · 2018-07-03T22:46:27Z

Fixes some issues brought up after this PR (#31215) was already merged. Also fixes issues related to indices options where they should be parameters rather than part of the request body.

elasticmachine · 2018-07-03T22:46:29Z

Pinging @elastic/es-core-infra

jdconrad · 2018-07-03T22:47:28Z

@javanna I believe I have addressed all the issues from the previous review.
@vladimirdolzhenko Thanks for pointing out the issues with indices options. I believe this PR fixes them.

vladimirdolzhenko

LGTM

vladimirdolzhenko

thanks @jdconrad - well done. I left few comments - there are really cosmetic ones

vladimirdolzhenko · 2018-07-04T07:17:59Z

...ain/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotResponse.java

@@ -38,6 +40,14 @@
 */
 public class CreateSnapshotResponse extends ActionResponse implements ToXContentObject {

+    private static final ObjectParser<CreateSnapshotResponse, Void> CREATE_SNAPSHOT_RESPONSE_PARSER =


i'd prefer a short name - just PARSER

vladimirdolzhenko · 2018-07-04T07:21:51Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java

@@ -2069,6 +2069,8 @@ public void testCreateSnapshot() throws IOException {
            expectedParams.put("wait_for_completion", waitForCompletion.toString());


I can't comment line 2065 here (it's 4 lines above)

Boolean waitForCompletion = randomBoolean(); - no any reason for autoboxing

I use the toString method from it for convenience.

vladimirdolzhenko · 2018-07-04T07:26:16Z

...ain/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotResponse.java

+
+    static {
+        CREATE_SNAPSHOT_RESPONSE_PARSER.declareObject(CreateSnapshotResponse::setSnapshotInfoFromBuilder,
+            SnapshotInfo.SNAPSHOT_INFO_PARSER, new ParseField("snapshot"));


i think it would be better to simplify SNAPSHOT_INFO_PARSER to just PARSER as we use class reference

I'm going to leave this one for now because there are two parsers in SnapshotInfo.

javanna

I left a question, thanks @jdconrad !

javanna · 2018-07-04T08:45:27Z

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

@@ -54,6 +55,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
        request.applyContentParser(p -> createSnapshotRequest.source(p.mapOrdered()));
        createSnapshotRequest.masterNodeTimeout(request.paramAsTime("master_timeout", createSnapshotRequest.masterNodeTimeout()));
        createSnapshotRequest.waitForCompletion(request.paramAsBoolean("wait_for_completion", false));
+        createSnapshotRequest.indicesOptions(IndicesOptions.fromRequest(request, createSnapshotRequest.indicesOptions()));


I am missing why we need to add support for these params as request parameters. Aren't they part of the request body for this API? Are they still supported in the body, meaning do we parse them back when reading the request body?

indicesOptions is never a part of request body (e.g. RequestConverters#deleteIndex) - the biggest question is here - shall we drop in from request itself (i have some doubt on that) and do follow rest api spec - or we have to maintain it properly and add to rest api spec to reflect it ?

@vladimirdolzhenko you are referring to delete index, but create snapshot is a different API . See its docs: https://www.elastic.co/guide/en/elasticsearch/reference/6.3/modules-snapshots.html#_snapshot . It seems like ignore_unavailable (which is one of the indices options), as well as index, are specified in the request body. That's why I am asking on what we intend to do here.

@javanna I see your point - quite inconsistency here - sorry @jdconrad to mislead you

@javanna @vladimirdolzhenko I like the consistency and methods available for switching to indices options the way delete index is doing it; however at the same time, it's probably not worth deprecating the current way right now. My instinct is to revert this to how it was for now, and we can decide if we want to switch them to parameters at a later time. Would that work for you guys?

yes that's a good idea. let's discuss API changes separately.

@javanna Will do. Thanks for the review here!
@vladimirdolzhenko No need to apologize! What you showed me makes a lot of sense for a future improvement.

jdconrad · 2018-07-10T17:02:35Z

@javanna I reverted the API changes, and I think this is good to go now. Would you please take a look when you get a chance? Thanks in advance!

jdconrad · 2018-07-10T19:05:23Z

@elasticmachine Test this please.

jdconrad · 2018-07-10T21:08:37Z

@elasticmachine Test this please.

javanna

I left one small comment LGTM otherwise

javanna · 2018-07-11T08:53:10Z

...main/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequest.java

@@ -433,8 +433,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        if (indicesOptions != null) {
            indicesOptions.toXContent(builder, params);
        }
-        builder.field("wait_for_completion", waitForCompletion);
-        builder.field("master_node_timeout", masterNodeTimeout.toString());


good catch , these should not be printed out, nor parsed back, they are query_string parameters.

Credit to @vladimirdolzhenko for pointing this out.

javanna · 2018-07-11T09:01:02Z

server/src/main/java/org/elasticsearch/action/support/IndicesOptions.java

+        builder.field("allow_no_indices", allowNoIndices());
+        builder.field("forbid_aliases_to_multiple_indices", allowAliasesToMultipleIndices() == false);
+        builder.field("forbid_closed_indices", forbidClosedIndices());
+        builder.field("ignore_aliases", ignoreAliases());


this one ends up sending parameters that are getting ignored, see IndicesOptions.fromMap. we should remove the last three parameters. This should be cleaned up, the problem is that some indices options are settable at REST, while some other are internal properties that define each API (the default argument in fromMap) which cannot be changed, so they should never be printed out nor parsed back.

Removed. Makes sense that we only print out what can be input.

jdconrad · 2018-07-13T16:59:59Z

@javanna Thanks for the review here! Will commit once the CI passes.

Make SnapshotInfo and CreateSnapshotResponse parsers lenient for backwards compatibility. Remove extraneous fields from CreateSnapshotRequest toXContent.

* es/6.x: Use correct formatting for links (#29460) Revert "Adds a new auto-interval date histogram (#28993)" Revert "fix typo" fix typo Adds a new auto-interval date histogram (#28993) [Rollup] Replace RollupIT with a ESRestTestCase version (#31977) [Rollup] Fix duplicate field names in test (#32075) [Tests] Fix failure due to changes exception message (#32036) [Test] Mute MlJobIT#testDeleteJobAfterMissingAliases Replace Ingest ScriptContext with Custom Interface (#32003) (#32060) Cleanup Duplication in `PainlessScriptEngine` (#31991) (#32061) HLRC: Add xpack usage api (#31975) Clean Up Snapshot Create Rest API (#31779)

jdconrad added 3 commits July 3, 2018 11:06

Follow up for SnapshotCreate rest client code based on PR feedback.

ddace8f

Fix parameters for indicesOptions.

1b903ce

Fix documentation.

c8f89d4

jdconrad added >enhancement v7.0.0 :Core/Java High Level REST Client v6.4.0 labels Jul 3, 2018

jdconrad requested review from vladimirdolzhenko and javanna July 3, 2018 22:46

vladimirdolzhenko approved these changes Jul 4, 2018

View reviewed changes

vladimirdolzhenko reviewed Jul 4, 2018

View reviewed changes

javanna reviewed Jul 4, 2018

View reviewed changes

jdconrad added 2 commits July 10, 2018 09:14

Merge branch 'master' into followup

746cd8d

Revert extraneous API changes.

9777fd1

javanna approved these changes Jul 11, 2018

View reviewed changes

jdconrad added 2 commits July 13, 2018 09:43

Merge branch 'master' into followup

a0f4347

Response to PR comments.

3fa1103

jdconrad merged commit 42ca520 into elastic:master Jul 13, 2018

jdconrad added the backport pending label Jul 13, 2018

jdconrad added a commit that referenced this pull request Jul 13, 2018

Clean Up Snapshot Create Rest API (#31779)

fc699c6

Make SnapshotInfo and CreateSnapshotResponse parsers lenient for backwards compatibility. Remove extraneous fields from CreateSnapshotRequest toXContent.

jdconrad removed the backport pending label Jul 13, 2018

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean Up Snapshot Create Rest API #31779

Clean Up Snapshot Create Rest API #31779

jdconrad commented Jul 3, 2018

elasticmachine commented Jul 3, 2018

jdconrad commented Jul 3, 2018

vladimirdolzhenko left a comment

vladimirdolzhenko left a comment

vladimirdolzhenko Jul 4, 2018

jdconrad Jul 10, 2018

vladimirdolzhenko Jul 4, 2018

jdconrad Jul 10, 2018

vladimirdolzhenko Jul 4, 2018

jdconrad Jul 10, 2018

javanna left a comment

javanna Jul 4, 2018

vladimirdolzhenko Jul 4, 2018

javanna Jul 4, 2018 •

edited

Loading

vladimirdolzhenko Jul 4, 2018

jdconrad Jul 4, 2018

javanna Jul 4, 2018

jdconrad Jul 4, 2018

jdconrad Jul 10, 2018

jdconrad commented Jul 10, 2018

jdconrad commented Jul 10, 2018 •

edited

Loading

jdconrad commented Jul 10, 2018

javanna left a comment

javanna Jul 11, 2018

jdconrad Jul 13, 2018

javanna Jul 11, 2018

jdconrad Jul 13, 2018

jdconrad commented Jul 13, 2018

		@@ -2069,6 +2069,8 @@ public void testCreateSnapshot() throws IOException {
		expectedParams.put("wait_for_completion", waitForCompletion.toString());

Clean Up Snapshot Create Rest API #31779

Clean Up Snapshot Create Rest API #31779

Conversation

jdconrad commented Jul 3, 2018

elasticmachine commented Jul 3, 2018

jdconrad commented Jul 3, 2018

vladimirdolzhenko left a comment

Choose a reason for hiding this comment

vladimirdolzhenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Jul 10, 2018

jdconrad commented Jul 10, 2018 • edited Loading

jdconrad commented Jul 10, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Jul 13, 2018

javanna Jul 4, 2018 •

edited

Loading

jdconrad commented Jul 10, 2018 •

edited

Loading