Add Restore Snapshot High Level REST API by danielmitterdorfer · Pull Request #32155 · elastic/elasticsearch

danielmitterdorfer · 2018-07-18T07:55:42Z

With this commit we add the restore snapshot API to the Java high level
REST client.

With this commit we add the restore snapshot API to the Java high level REST client. Relates elastic#27205

elasticmachine · 2018-07-18T07:55:44Z

Pinging @elastic/es-core-infra

nik9000 · 2018-07-18T16:13:16Z

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

+
+        RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repository, snapshot);
+        setRandomMasterTimeout(restoreSnapshotRequest, expectedParams);
+        Boolean waitForCompletion = randomBoolean();


boolean instead? looks like it can't be null.

Why not just a simple

if (randomBoolean()) { restoreSnapshotRequest.waitForCompletion(true); expectedParams.... }

yeah, that is better I think.

Honestly, I tried to be consistent with other tests (in that case testCreateSnapshot) but I changed it now accordingly.

nik9000 · 2018-07-18T16:15:51Z

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

+        CreateSnapshotResponse createSnapshotResponse = createTestSnapshot(createSnapshotRequest);
+        assertEquals(RestStatus.OK, createSnapshotResponse.status());
+
+        RestoreSnapshotRequest request = new RestoreSnapshotRequest(testRepository, testSnapshot);


Delete the index and check that it came back? Stick a document in it and check that it was restored? I don't want to test snapshot/restore but I do want to get a fairly real test just in case.

@nik9000 Do you not feel that restoring showing successful shards > 0 is enough here? Its a rest api call after all, and it might be out of scope to test this at the rest api level.

Yeah, I suppose seeing >0 successful shards is enough. I'd still like to see that index deleted first.

The test deletes the index now before attempting to restore it.

nik9000 · 2018-07-18T16:17:32Z

...in/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java

                if (entry.getValue() instanceof String) {
                    renamePattern((String) entry.getValue());
-                } else {
+                } else if (entry.getValue() != null) {


Should we instead skip emitting the value if it is null?

if im reading this right, it will only fail if its not a string and is not null. Otherwise itll emit null below in the toXContent. I think the null checks would make sense in toXContent as well, but we certainly dont want a non-string being emitted, so i think the IAE is ok here.

@hub-cap you're reading it right. However, I decided to revert to the old behavior in this method and skip emitting renamePattern / renameReplacement now in #toXContent() when they are null.

nik9000 · 2018-07-18T16:18:15Z

...in/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java

+
+    @Override
+    public String toString() {
+        return "RestoreSnapshotRequest{" +


Strings.toString is pretty good for this. It spits it out in json.

Ah, good catch. I didn't know it exists. Btw, I implemented this consistently with other classes (i.e. CreateSnapshotRequest). I wonder whether we should bite the bullet and implement this consistently across all implementations of ToXContent (that also implement as of today)?

nik9000 · 2018-07-18T16:20:44Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

    }
+
+    public static RestoreSnapshotResponse fromXContent(XContentParser parser) throws IOException {
+        RestoreSnapshotResponse response = new RestoreSnapshotResponse();


I feel like ObjectParser may be better here just so I don't have to think "does this thing really only ever spit out one fields? does this code properly ignore unknown fields so it is forward compatible?"

We have this in another spot too, which is why i think @danielmitterdorfer wrote it this way. Its probably good to change it in both spots (CreateSnapshot). Maybe in a separate PR?

@hub-cap correct; I implemented this identically to CreateSnapshotResponse. Similarly to RestoreInfo there is a mismatch between the object structure and the XContent structure. For RestoreInfo I managed to use ObjectParser but I had a hard time figuring out how to make it work. I also agree that if we change it here, we should change it in CreateSnapshotResponse as well.

This definitely does not ignore unknown fields. It will fail if snapshot or accepted is not in the body. @nik9000 how should we handle this? We can go back to the older for loop style of these, or just fix it here using object parser. Id prefer the latter given @danielmitterdorfer has infinite time (which i dont know).

Failing on less stuff is fine, but we should indeed ignore unknown fields. I think ConstructingObjectParser would work fine for this if you declare shapshot as an optionalConstructorArg and you declare accepted as boolean optionalConstructorArg and throw it on the floor when building the actual object. It isn't super efficient but it will properly handle unknown fields and it isn't too bad.

And it means we don't need to think too hard about the parsing. Which I don't think is worth it for an API as low traffic as this.

nik9000 · 2018-07-18T16:22:45Z

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

+    }
+
+    @Override
+    public String toString() {


Same thing about toString as above. Either way is fine with me, but we're starting to standardize on using the json.

Thanks. Changed.

nik9000 · 2018-07-18T16:25:16Z

...va/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java

+import java.util.List;
+import java.util.Map;
+
+public class RestoreSnapshotRequestTests extends ESTestCase {


We have been using AbstractStreamableTestCase for these tests lately and I think it forces us to test important things.

Like whether or not we support adding additional fields and reordering fields.

I used AbstractWireSerializingTestCase as a base class because AbstractStreamableTestCase requires the test subject to be Streamable but RestoreSnapshotRequest raises an UnsupportedOperationException ("usage of Streamable is to be replaced by Writeable") in #readFrom(StreamInput).

nik9000 · 2018-07-18T16:26:25Z

...a/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponseTests.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class RestoreSnapshotResponseTests extends AbstractXContentTestCase<RestoreSnapshotResponse> {


I think this might should also be a AbstractStreamableTestCase as well.

The tests that test a class that impl fromXContent have been moving toward this. It was a newer test class created in cd3d9c9. I think its valid for this, but not valid for the request test above that only impls a toXContent

Fine by me.

hub-cap · 2018-07-18T17:06:15Z

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

+
+        RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repository, snapshot);
+        setRandomMasterTimeout(restoreSnapshotRequest, expectedParams);
+        Boolean waitForCompletion = randomBoolean();


Why not just a simple

if (randomBoolean()) { restoreSnapshotRequest.waitForCompletion(true); expectedParams.... }

hub-cap · 2018-07-18T17:07:03Z

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

+            expectedParams.put("wait_for_completion", waitForCompletion.toString());
+        }
+        if (randomBoolean()) {
+            restoreSnapshotRequest.masterNodeTimeout("120s");


randomTimeValue()

Good idea; didn't know it existed. I changed it.

hub-cap · 2018-07-18T17:07:51Z

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

+        CreateSnapshotResponse createSnapshotResponse = createTestSnapshot(createSnapshotRequest);
+        assertEquals(RestStatus.OK, createSnapshotResponse.status());
+
+        RestoreSnapshotRequest request = new RestoreSnapshotRequest(testRepository, testSnapshot);


@nik9000 Do you not feel that restoring showing successful shards > 0 is enough here? Its a rest api call after all, and it might be out of scope to test this at the rest api level.

hub-cap · 2018-07-18T18:02:46Z

...in/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java

                if (entry.getValue() instanceof String) {
                    renamePattern((String) entry.getValue());
-                } else {
+                } else if (entry.getValue() != null) {


if im reading this right, it will only fail if its not a string and is not null. Otherwise itll emit null below in the toXContent. I think the null checks would make sense in toXContent as well, but we certainly dont want a non-string being emitted, so i think the IAE is ok here.

hub-cap · 2018-07-18T18:04:16Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

    }
+
+    public static RestoreSnapshotResponse fromXContent(XContentParser parser) throws IOException {
+        RestoreSnapshotResponse response = new RestoreSnapshotResponse();


We have this in another spot too, which is why i think @danielmitterdorfer wrote it this way. Its probably good to change it in both spots (CreateSnapshot). Maybe in a separate PR?

hub-cap · 2018-07-18T18:10:47Z

...a/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponseTests.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class RestoreSnapshotResponseTests extends AbstractXContentTestCase<RestoreSnapshotResponse> {


The tests that test a class that impl fromXContent have been moving toward this. It was a newer test class created in cd3d9c9. I think its valid for this, but not valid for the request test above that only impls a toXContent

danielmitterdorfer · 2018-07-19T07:17:15Z

@hub-cap, @nik9000 thanks for your reviews! I addressed your comments now in a79f8c2. Can you please have another look?

…shot

nik9000 · 2018-07-20T21:26:28Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

+            parser.nextToken(); // move to 'accepted' field value
+
+            if (parser.booleanValue()) {
+                // ensure accepted is a boolean value


I think it is weird to just drop this on the floor. But we really don't have anywhere to put it.....

Yes, this is caused by the (already existing) impedance mismatch of XContent and object structure which honestly causes quite some headaches.

nik9000 · 2018-07-20T21:31:11Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

    }
+
+    public static RestoreSnapshotResponse fromXContent(XContentParser parser) throws IOException {
+        RestoreSnapshotResponse response = new RestoreSnapshotResponse();


Failing on less stuff is fine, but we should indeed ignore unknown fields. I think ConstructingObjectParser would work fine for this if you declare shapshot as an optionalConstructorArg and you declare accepted as boolean optionalConstructorArg and throw it on the floor when building the actual object. It isn't super efficient but it will properly handle unknown fields and it isn't too bad.

nik9000 · 2018-07-20T21:31:35Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

    }
+
+    public static RestoreSnapshotResponse fromXContent(XContentParser parser) throws IOException {
+        RestoreSnapshotResponse response = new RestoreSnapshotResponse();


And it means we don't need to think too hard about the parsing. Which I don't think is worth it for an API as low traffic as this.

danielmitterdorfer · 2018-07-24T03:27:40Z

@hub-cap, @nik9000 with the most recent commit, RestoreSnapshotResponse supports unknown fields as well now. Can you please have another look?

nik9000

LGTM

nik9000 · 2018-07-24T13:31:04Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

-            throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "], expected ['}']");
-        }
+    public static final ConstructingObjectParser<RestoreSnapshotResponse, Void> PARSER = new ConstructingObjectParser<>(
+        "restore_snapshot", true, v -> {


Could you indent this one more level so it doesn't read like it is part of the block below?

nik9000 · 2018-07-24T13:32:33Z

Thanks for making all of the changes that I asked for!

danielmitterdorfer · 2018-07-24T13:48:21Z

Thanks for your help @nik9000!

With this commit we add the restore snapshot API to the Java high level REST client. Relates #27205 Relates #32155

danielmitterdorfer · 2018-07-26T15:37:04Z

Backported to 6.x in 92d7802.

Add Restore Snapshot High Level REST API

ba0618d

With this commit we add the restore snapshot API to the Java high level REST client. Relates elastic#27205

danielmitterdorfer added >enhancement review v7.0.0 v6.4.0 labels Jul 18, 2018

danielmitterdorfer requested a review from hub-cap July 18, 2018 07:55

javanna mentioned this pull request Jul 18, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

nik9000 reviewed Jul 18, 2018

View reviewed changes

hub-cap reviewed Jul 18, 2018

View reviewed changes

Address review comments

a79f8c2

Merge remote-tracking branch 'origin/master' into client-restore-snap…

aa6da63

…shot

nik9000 reviewed Jul 20, 2018

View reviewed changes

Support unknown fields in RestoreSnapshotResponse

04d8542

nik9000 approved these changes Jul 24, 2018

View reviewed changes

Improve formatting

f48ca63

danielmitterdorfer merged commit 73a3889 into elastic:master Jul 24, 2018

danielmitterdorfer added backport pending and removed review labels Jul 24, 2018

danielmitterdorfer deleted the client-restore-snapshot branch July 24, 2018 14:20

danielmitterdorfer added v6.5.0 and removed v6.4.0 labels Jul 24, 2018

danielmitterdorfer added a commit that referenced this pull request Jul 26, 2018

Add Restore Snapshot High Level REST API

92d7802

With this commit we add the restore snapshot API to the Java high level REST client. Relates #27205 Relates #32155

danielmitterdorfer removed the backport pending label Jul 26, 2018

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

Conversation

danielmitterdorfer commented Jul 18, 2018

Uh oh!

elasticmachine commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielmitterdorfer Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielmitterdorfer commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danielmitterdorfer Jul 19, 2018 •

edited

Loading