Add Create Snapshot to High-Level Rest Client #31215

jdconrad · 2018-06-08T18:28:33Z

Added support to the high-level rest client for the create snapshot API call. This required several changes to toXContent which may need to be cleaned up in a later PR. Also added several parsers for fromXContent to be able to retrieve appropriate responses along with tests.

elasticmachine · 2018-06-08T18:28:34Z

Pinging @elastic/es-distributed

elasticmachine · 2018-06-08T18:28:36Z

Pinging @elastic/es-core-infra

incosistency.

hub-cap

great work! i hope we can add some more tests to the new functionality. Small changes in the review.

hub-cap · 2018-06-13T14:18:17Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

@@ -816,6 +817,19 @@ static Request verifyRepository(VerifyRepositoryRequest verifyRepositoryRequest)
        return request;
    }

+    static Request createSnapshot(CreateSnapshotRequest createSnapshotRequest) throws IOException {


There are a few extra options i see in the transport layer. Lets make sure we account for all these params / optional things.

https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/create/TransportCreateSnapshotAction.java#L76-L82

All of these should be covered as they end up as part of the content body including partial, indices, global_state, and settings.

hub-cap · 2018-06-13T14:20:11Z

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

+
+    private CreateSnapshotResponse createTestSnapshot(String repository, String snapshot) throws IOException {
+        // assumes the repository already exists
+        CreateSnapshotRequest request = new CreateSnapshotRequest(repository, snapshot);


I think we should add more of the options here, randomly with some if randomBoolean()

Added several random booleans for options where it made sense.

hub-cap · 2018-06-13T14:22:17Z

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

@@ -407,6 +409,28 @@ public CreateSnapshotRequest source(Map<String, Object> source) {
        return this;
    }

+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


is there any test for this?

Added tests.

hub-cap · 2018-06-13T14:23:25Z

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

+    public static CreateSnapshotResponse fromXContent(XContentParser parser) throws IOException {
+        CreateSnapshotResponse createSnapshotResponse = new CreateSnapshotResponse();
+
+        parser.nextToken(); // '{'


This seems like we should be using a nested parser here. Similar to how this works, https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java#L112-L116

Misread the location of this. Let me see what I can do.

After reviewing the structure of the content here, I do not believe it makes sense to add a new object parser in this case since it would end up being more obfuscated for no reason since with one of the fields it's an either or and the field doesn't need to actually be parsed (it's used for user-consumable output only). This would mean additional empty methods. It's better to call fromXContent directly on SnapshotInfo. I did, however, add several checks to ensure the proper tokens are being read.

hub-cap · 2018-06-13T14:23:37Z

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

+
+    @Override
+    public int hashCode() {
+


remove empty line

Intellij added this automatically.

hub-cap · 2018-06-13T14:28:04Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java

@@ -79,6 +84,170 @@
    private static final Comparator<SnapshotInfo> COMPARATOR =
        Comparator.comparing(SnapshotInfo::startTime).thenComparing(SnapshotInfo::snapshotId);

+    private static final class SnapshotInfoBuilder {


is it possible to test all this new stuff?

This should get full coverage from the existing SnapshotResponseTests.

hub-cap · 2018-06-13T14:28:39Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java

@@ -448,12 +609,20 @@ private XContentBuilder toXContentSnapshot(final XContentBuilder builder, final
        return builder;
    }

+    public static SnapshotInfo fromXContent(final XContentParser parser) throws IOException {
+        parser.nextToken();


maybe validate that these tokens are thee right start_object or whatever ?

danielmitterdorfer · 2018-06-18T04:58:08Z

...evel/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java

+    }
+
+
+    public void testCreateSnapshotRepositoryAsync() throws InterruptedException {


Nit: This should probably be called testCreateSnapshotAsync() (without "repository" in the name)

jdconrad · 2018-06-26T23:41:35Z

@hub-cap Thanks for the initial review! Ready for the next round. Added requested tests and parsing checks where applicable. Also given that some of the parsing code is legacy, I do not believe adding an extra object parser made sense in this case to CreateSnapshotResponse -- left more information under that comment.

hub-cap

one minor nit once added

hub-cap · 2018-06-27T13:52:21Z

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

+        String endpoint = "/_snapshot/" + repository + "/" + snapshot;
+
+        CreateSnapshotRequest createSnapshotRequest = new CreateSnapshotRequest(repository, snapshot);
+        setRandomMasterTimeout(createSnapshotRequest, expectedParams);


can u also add a set to the withWaitForCompletion

Good call, added.

jdconrad · 2018-06-27T14:25:17Z

@baz - Thanks for the follow up! Will commit as soon as CI passes.

* elastic/master: (57 commits) HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (elastic#31215) Remove legacy MetaDataStateFormat (elastic#31603) Add explain API to high-level REST client (elastic#31387) Preserve thread context when connecting to remote cluster (elastic#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (elastic#31494) [TEST] call yaml client close method from test suite (elastic#31591) ingest: Add ignore_missing property to foreach filter (elastic#22147) (elastic#31578) Fix a formatting issue in the docvalue_fields documentation. (elastic#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (elastic#31575) Docs: Clarify sensitive fields watcher encryption (elastic#31551) Watcher: Remove never executed code (elastic#31135) Add support for switching distribution for all integration tests (elastic#30874) Improve robustness of geo shape parser for malformed shapes (elastic#31449) QA: Create xpack yaml features (elastic#31403) Improve test times for tests using `RandomObjects::addFields` (elastic#31556) ...

Tim-Brooks · 2018-06-28T00:12:16Z

@jdconrad Please ping when this is back ported. It will probably work best for me to back port mine (get snapshots) after this is back ported.

javanna · 2018-06-28T15:30:57Z

...evel/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java

+        // end::create-snapshot-execute
+
+        // tag::create-snapshot-response
+        RestStatus status = response.status(); // <1>


@jdconrad can you please expand on what can be done with the response other than looking at the status? We also return the SnapshotInfo right? That seems like an interesting thing to document for users.

javanna · 2018-06-28T15:34:13Z

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

@@ -45,6 +48,10 @@
    CreateSnapshotResponse() {
    }

+    void setSnapshotInfo(SnapshotInfo snapshotInfo) {


Can we use the existing constructor in the only place where this is called (test) and remove this new method?

Done.

Edit: Actually, going to have a similar method and use an ObjectParser for CreateSnapshotResponse.

javanna · 2018-06-28T15:39:17Z

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

+
+            parser.nextToken(); // move past 'true'/'false'
+        } else {
+            throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "] expected ['snapshot', 'accepted']");


parsing here is very strict. while strictness is generally a good thing, it hinders forward compatibility in this specific case when parsing back responses in the client. That's why parsing should also be tested with supportsUnknownFields set to true unless there are good reasons not to do so.
What if one day we add a new field to the response, which is a perfectly backwards compatible change? The client would break with such a response, while it should ignore the additional fields till the newer client is used, which will be able to read such new field.

Switched to a lenient ObjectParser.

javanna · 2018-06-28T15:40:50Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java

+            new ObjectParser<>(SnapshotInfoBuilder.class.getName(), SnapshotInfoBuilder::new);
+
+    private static final ObjectParser<ShardStatsBuilder, Void> SHARD_STATS_PARSER =
+        new ObjectParser<>(ShardStatsBuilder.class.getName(), ShardStatsBuilder::new);


these objects parser should be made to ignore unknown fields

@tbrooks8 Thanks for doing this already!

* master: Docs: Remove duplicate test setup Print output when the name checker IT fails (#31660) Fix syntax errors in get-snapshots docs (#31656) Docs: Fix description of percentile ranks example example (#31652) Add MultiSearchTemplate support to High Level Rest client (#30836) Add test for low-level client round-robin behaviour (#31616) SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Correct integTest enable logic (#31646) Fix missing get-snapshots docs reference #31645 Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) Add Get Snapshots High Level REST API (#31537) QA: Merge query-builder-bwc to restart test (#30979) Update reindex.asciidoc (#31626) Docs: Skip xpack snippet tests if no xpack (#31619) mute CreateSnapshotRequestTests HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (#31215) Remove legacy MetaDataStateFormat (#31603) Add explain API to high-level REST client (#31387) Preserve thread context when connecting to remote cluster (#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (#31494) [TEST] call yaml client close method from test suite (#31591)

jdconrad · 2018-06-28T16:36:31Z

@tbrooks8 Will let you know. I need to fix some tests before I can backport this specific change (#31630)
@javanna Will follow up. Thanks for the review!

javanna · 2018-06-28T17:14:00Z

one more thing @jdconrad can you rename the methods to just create. That's how it is defined in our REST spec.

jdconrad · 2018-06-28T17:41:08Z

@javanna Sure, do you mean the methods in RequestConverters or in some of the tests?

Added support to the high-level rest client for the create snapshot API call. This required several changes to toXContent which may need to be cleaned up in a later PR. Also added several parsers for fromXContent to be able to retrieve appropriate responses along with tests.

jdconrad · 2018-06-28T21:17:49Z

@tbrooks8 My backports are in. Will follow up on @javanna comments soon in a new PR.

javanna · 2018-06-28T21:17:53Z

@jdconrad the ones in SnapshotClient, the others could be adapted but are not really exposed to users, not as much the client methods.

* 6.x: Do not check for object existence when deleting repository index files (#31680) Remove extra check for object existence in repository-gcs read object (#31661) [Test] Clean up some repository-s3 tests (#31601) Merge AzureStorageService and AzureStorageServiceImpl and clean up tests (#31607) [Docs] Use capital letters in section headings (#31678) [DOCS] Add PQL language Plugin (#31237) [DOCS] Fix licensing API details (#31667) Fix CreateSnapshotRequestTests Failure (#31630) Add Create Snapshot to High-Level Rest Client (#31215) Add MultiSearchTemplate support to High Level Rest client (#31662) SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622) [TEST] Mute failing NamingConventionsTaskIT tests [DOCS] Replace CONFIG_DIR with ES_PATH_CONF (#31635) Add test for low-level client round-robin behaviour (#31616) HLRest: Fix test for explain API Add explain API to high-level REST client (#31387)

jdconrad added 11 commits June 6, 2018 17:31

Progress on parsers.

92acd76

Parsing successful with tests.

77cb270

More progress.

346fb6a

More progress.

13144c9

Merge branch 'master' into snapshotclient2

77fc66b

More progress.

baa4d49

Merge branch 'master' into snapshotclient2

5ebf689

More progress.

84b6e40

Merge branch 'master' into snapshotclient2

a1e50d5

Added docs tests.

2a7665c

Docs fixes.

9879a28

jdconrad added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 :Core/Java High Level REST Client v6.4.0 labels Jun 8, 2018

jdconrad requested a review from hub-cap June 8, 2018 18:28

hub-cap added >enhancement and removed :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Jun 8, 2018

jdconrad added 4 commits June 8, 2018 11:35

Style violation.

891b2a0

Fix test failure.

04b568e

Fix discrepanices in equals method for ShardId related to uuid of index

c664596

incosistency.

Merge branch 'master' into snapshotclient2

5ea58ec

javanna mentioned this pull request Jun 12, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

jdconrad requested a review from javanna June 12, 2018 19:21

hub-cap suggested changes Jun 13, 2018

View reviewed changes

Merge branch 'master' into snapshotclient2

226339c

danielmitterdorfer reviewed Jun 18, 2018

View reviewed changes

danielmitterdorfer mentioned this pull request Jun 18, 2018

Add Delete Snapshot High Level REST API #31393

Merged

jdconrad added 3 commits June 26, 2018 11:22

Merge branch 'master' into snapshotclient2

760ed40

Response to PR feedback.

7aaaf95

Merge branch 'master' into snapshotclient2

3122f47

jdconrad removed the request for review from javanna June 26, 2018 23:42

hub-cap approved these changes Jun 27, 2018

View reviewed changes

jdconrad added 2 commits June 27, 2018 07:11

Merge branch 'master' into snapshotclient2

0aaa589

Added waitForCompletion to RequestConvertTests.

ffbf58d

jdconrad merged commit 61eefc8 into elastic:master Jun 27, 2018

jdconrad added the backport pending label Jun 27, 2018

vladimirdolzhenko mentioned this pull request Jun 27, 2018

[CI] CreateSnapshotRequestTests fails #31625

Closed

javanna reviewed Jun 28, 2018

View reviewed changes

jdconrad removed the backport pending label Jun 28, 2018

jdconrad mentioned this pull request Jul 3, 2018

Clean Up Snapshot Create Rest API #31779

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

		}


		public void testCreateSnapshotRepositoryAsync() throws InterruptedException {

Add Create Snapshot to High-Level Rest Client #31215

Add Create Snapshot to High-Level Rest Client #31215

Conversation

jdconrad commented Jun 8, 2018

elasticmachine commented Jun 8, 2018

elasticmachine commented Jun 8, 2018

hub-cap 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

Choose a reason for hiding this comment

jdconrad Jun 26, 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

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 Jun 26, 2018

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Jun 27, 2018

Tim-Brooks commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad Jul 3, 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

jdconrad commented Jun 28, 2018

javanna commented Jun 28, 2018

jdconrad commented Jun 28, 2018

jdconrad commented Jun 28, 2018 • edited Loading

javanna commented Jun 28, 2018

jdconrad Jun 26, 2018 •

edited

Loading

jdconrad Jul 3, 2018 •

edited

Loading

jdconrad commented Jun 28, 2018 •

edited

Loading