HLRC: migration api - upgrade #34898

pgomulka · 2018-10-26T09:39:54Z

Implement high level client for migration upgrade API. It should wrap RestHighLevelClient and expose high level IndexUpgradeRequest (new) & BulkByScrollResponse (already used) objects.

endpoints:
sync & async:
POST /_xpack/migration/upgrade/<index_name>
sync with task api:
POST /_xpack/migration/upgrade/.watches?wait_for_completion=false

links:
Documentation reference:
https://www.elastic.co/guide/en/elasticsearch/reference/current/migration-api-assistance.html
X-pack API:
https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/src/test/resources/rest-api-spec/api/xpack.migration.get_assistance.json

refers: #29827

elasticmachine · 2018-10-26T09:41:58Z

Pinging @elastic/es-core-infra

pgomulka · 2018-10-30T16:08:34Z

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

+            BulkByScrollResponse::fromXContent, Collections.emptySet());
+    }
+
+    public IndexUpgradeSubmissionResponse submitUpgradeTask(IndexUpgradeRequest request, RequestOptions options) throws IOException {


as discussed, I want to refactor that later to return TaskSubmissionResponse and use across other methods where wait_for_completion=false was used

there is another PR where we discussed if it wouldn't be better to return just TaskID:
https://github.com/elastic/elasticsearch/pull/35202/files
I might get back to this one and refactor to this TaskID

pgomulka · 2018-10-30T16:09:35Z

...vel/src/test/java/org/elasticsearch/client/documentation/MigrationClientDocumentationIT.java

@@ -80,4 +92,72 @@ public void testGetAssistance() throws IOException {
        }
        // end::get-assistance-response
    }
+
+    public void testUpgrade() throws IOException {


here I am adding these test cases which are duplicates of the one I have in MigrationIT. I made it more documentation friendly here though.

pgomulka · 2018-10-30T16:11:19Z

...k/protocol/src/main/java/org/elasticsearch/protocol/xpack/migration/IndexUpgradeRequest.java

+import java.util.Arrays;
+import java.util.Objects;
+
+public class IndexUpgradeRequest extends MasterNodeReadRequest<IndexUpgradeRequest> implements IndicesRequest {


this request allows only one index to be used - as per rest spec - however the interface returns arrays. might be a bit confusing..

pgomulka · 2018-10-30T16:13:20Z

...k/protocol/src/main/java/org/elasticsearch/protocol/xpack/migration/IndexUpgradeRequest.java

+        indices = new String[]{index};
+    }
+
+    public IndexUpgradeRequest(StreamInput in) throws IOException {


to be safe, I was looking into how IndexUpgradeInfo was implemented, copied that class and removed some methods that were not needed. Not sure if we really need this constructor. This is probably how this could be used on the server side, but that class is intended to be used on client side only. Should I remove it too?

pgomulka · 2018-10-30T16:13:44Z

...src/main/java/org/elasticsearch/protocol/xpack/migration/IndexUpgradeSubmissionResponse.java

+/**
+ * Response object that contains the taskID from submitted IndexUpgradeRequest
+ */
+public class IndexUpgradeSubmissionResponse extends ActionResponse implements ToXContentObject {


as mentioned earlier - this will be refactored later

pgomulka · 2018-10-30T16:14:23Z

@hub-cap would you like to have a look into this one?

Implement high level client for migration upgrade API. It should wrap RestHighLevelClient and expose high level IndexUpgradeRequest (new), IndexTaskResponse for submissions with wait_for_completion=false and BulkByScrollResponse (already used) objects. refers: elastic#29827

pgomulka · 2018-10-30T16:31:48Z

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

-                return putParam("wait_for_completion", Boolean.TRUE.toString());
-            }
-            return this;
+            return putParam("wait_for_completion", Boolean.toString(waitForCompletion));


that need to be to set false and pass through to the server.

Ahh, this is due to the fact that wait_for_completion defaults to true in this API, right?

exactly. See RestIndexUpgradeAction:60
if (request.paramAsBoolean("wait_for_completion", true)) {

pgomulka · 2018-10-30T16:33:21Z

docs/java-rest/high-level/migration/upgrade.asciidoc

@@ -0,0 +1,79 @@
+[[java-rest-high-migration-upgrade]]


there is a lot of duplication between this and for instance bulk api doc. Probably some refactoring to remove doc duplication would be welcome

yea i think that we can wait till docs squares away what they are doing with our rest api docs before diving in to this.

hub-cap

Great work, getting close to finishing this! Thanks for all the thought around the task work too. Some nits mostly and the test comment below.

Please have a look at the test cases that recent HLRC APIs have done for the response objects. Lets add one for the submission response.

hub-cap · 2018-11-02T14:00:03Z

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

+            BulkByScrollResponse::fromXContent, Collections.emptySet());
+    }
+
+    public IndexUpgradeSubmissionResponse submitUpgradeTask(IndexUpgradeRequest request, RequestOptions options) throws IOException {


hub-cap · 2018-11-02T14:03:10Z

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

-                return putParam("wait_for_completion", Boolean.TRUE.toString());
-            }
-            return this;
+            return putParam("wait_for_completion", Boolean.toString(waitForCompletion));


Ahh, this is due to the fact that wait_for_completion defaults to true in this API, right?

hub-cap · 2018-11-02T14:03:34Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+import java.util.Arrays;
+import java.util.Objects;
+
+public class IndexUpgradeRequest extends MasterNodeReadRequest<IndexUpgradeRequest> implements IndicesRequest {


some javadoc here pls

hub-cap · 2018-11-02T14:05:01Z

...h-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeSubmissionResponse.java

+        this.task = task;
+    }
+
+


remove a space

hub-cap · 2018-11-02T14:06:41Z

...h-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeSubmissionResponse.java

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


We can remove this.

hub-cap · 2018-11-02T14:10:30Z

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

                new IndexUpgradeInfoRequest("test"), RequestOptions.DEFAULT);
            assertEquals(0, response.getActions().size());
        }
    }
+
+    public void testUpgrade() throws IOException {


This seems like a failure case in the test itself but the name of the method does not seem to reflect that. Something like testUpgradeIndexCannotBeUpgraded (feel free to name it yourself, im not a stickler on names).

good idea. Sadly this is not possible to test easily the real upgrade

hub-cap · 2018-11-02T14:11:24Z

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

+    private BooleanSupplier checkCompletionStatus(IndexUpgradeSubmissionResponse upgrade) {
+        return () -> {
+            try {
+                Response response = client().performRequest(new Request("GET", "/_tasks/" + upgrade.getTask().toString()));


pls put a note saying why this is low level rest (the tasks api has not been finished yet etc).

will do- will leave a TODO note to revisit once the high level api for getTaskByID is finished

hub-cap · 2018-11-02T14:13:51Z

...vel/src/test/java/org/elasticsearch/client/documentation/MigrationClientDocumentationIT.java

@@ -80,4 +92,72 @@ public void testGetAssistance() throws IOException {
        }
        // end::get-assistance-response
    }
+
+    public void testUpgrade() throws IOException {


hub-cap · 2018-11-02T14:14:10Z

...vel/src/test/java/org/elasticsearch/client/documentation/MigrationClientDocumentationIT.java

+            BulkByScrollResponse response = client.migration().upgrade(request, RequestOptions.DEFAULT);
+            // end::upgrade-execute
+
+        }catch(ElasticsearchStatusException e){


space }catch(

hub-cap · 2018-11-02T14:15:05Z

docs/java-rest/high-level/migration/upgrade.asciidoc

@@ -0,0 +1,79 @@
+[[java-rest-high-migration-upgrade]]


yea i think that we can wait till docs squares away what they are doing with our rest api docs before diving in to this.

pgomulka · 2018-11-05T10:04:49Z

...el/src/test/java/org/elasticsearch/client/migration/IndexUpgradeSubmissionResponseTests.java

+
+import static org.elasticsearch.test.AbstractXContentTestCase.xContentTester;
+
+public class IndexUpgradeSubmissionResponseTests extends ESTestCase {


this test will be refactor once we introduce a common task submission response class. Likely it would be just TaskId

pgomulka · 2018-11-05T10:05:40Z

@hub-cap updated the PR with review followup. Would you be able to have a look again?

hub-cap

Getting close. Great work with the thought behind the sync/async/submit stuff.

hub-cap · 2018-11-05T17:58:35Z

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

+            IndexUpgradeSubmissionResponse::fromXContent, Collections.emptySet());
+    }
+
+


hub-cap · 2018-11-05T18:00:50Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+    }
+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {


Is this used anywhere?

no, removing

hub-cap · 2018-11-05T18:00:53Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+        indices = new String[]{index};
+    }
+
+    public IndexUpgradeRequest(StreamInput in) throws IOException {


Is this used anywhere?

nope, removing

hub-cap · 2018-11-05T18:01:24Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+ * A request for performing Upgrade on Index
+ * Part of Migration API
+ */
+public class IndexUpgradeRequest extends MasterNodeReadRequest<IndexUpgradeRequest> implements IndicesRequest {


I think this should be just a TimedRequest instead of a MasterNodeReadRequest

agree, great suggestion

hub-cap · 2018-11-05T18:01:36Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+    }
+
+    @Override
+    public ActionRequestValidationException validate() {


we can remove this once we make it a TimedRequest

hub-cap · 2018-11-05T18:03:02Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+    }
+
+    @Override
+    public void readFrom(StreamInput in) throws IOException {


is this used anywhere?

no, removed

hub-cap · 2018-11-05T18:03:55Z

...h-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeSubmissionResponse.java

+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
+import org.elasticsearch.common.xcontent.ObjectParser;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.tasks.TaskId;


Do we need a TaskId? Can we just use a string here instead? Id love to not pull things in from server here if possible

true, that is not ideal that it uses a class from server, but maybe we could copy or make our own TaskId class in HLRC?
I am a big fan of strong typed apis, and that abstracts nicely the concept of task identifier. I could refactor this class now, but since this is used in reindex and in getTask PRs I would prefer to refactor later

IMHO if we always pass it to server as a single string and never need to modify it or make decisions based on some portion of the ID (as i understand it has > 1 bit of information in the string separated by a delimiter), then making it a string is fine by me.

That being said, if it is easier for a user to create a object with those 3 values rather than just passing in a string, Im all for using an object.

I dont know all the uses, but im a fan of keeping it simple. If a user is never going to do anything with those 3 different parts of the string, and they only store it and give it back to us as a single string, it does not make much sense in my mind to create a class representation for the 3 strings. Ofc it might make sense to have this on the server, since those 3 strings are useful as individual pieces :D

All that being said, im ok with duplicating a TaskId class in HLRC. Im not too picky :)

What @hub-cap said. I too am unclear on the uses of TaskIDs and whether clients would need to break out component parts. I suppose even if they don't there's a potential upside to a typed object just because we could make them have private constructors so a client would only ever be able to give us IDs we'd previously given them, rather than any arbitrary string of their choosing.

Im still leaning toward string. we have to have a way to deserialize bits from the wire into a TaskId, so there will be a way for a user to create their own TaskId, if they want to go that far.

I am convinced too. Will refactor this to use TaskSubmissionResponse with taskId field of type String

Agreed. In retrospect, typed TaskIDs might only be useful if the world ran on Java but Java apps will typically hand off control to a Javascript front-end where it would become untyped strings in subsequent comms.

hub-cap · 2018-11-05T18:04:25Z

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

+    private BooleanSupplier checkCompletionStatus(IndexUpgradeSubmissionResponse upgrade) {
+        return () -> {
+            try {
+                Response response = client().performRequest(new Request("GET", "/_tasks/" + upgrade.getTask().toString()));


hub-cap

Looks like the docs can be DRY'd up (see #34925), and Id like to get the String VS TaskId issue solved, but then this will be good to merge.

hub-cap · 2018-11-06T18:00:30Z

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

+
+    private static Request prepareMigrateRequest(IndexUpgradeRequest indexUpgradeRequest, boolean waitForCompletion) {
+        RequestConverters.EndpointBuilder endpointBuilder = new RequestConverters.EndpointBuilder()
+            .addPathPartAsIs("_xpack/migration/upgrade")


you can comma separate these instead... i know the likelihood of us not using / is low, but its best to not have them in this.

hub-cap · 2018-11-06T18:01:35Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+ * A request for performing Upgrade on Index
+ * Part of Migration API
+ */
+


remove empty line

hub-cap · 2018-11-06T18:02:33Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

+ * Part of Migration API
+ */
+
+public class IndexUpgradeRequest extends TimedRequest implements IndicesRequest {


do we need this IndicesRequest? We know that we should provide a getter for the IndicesOptions so im ok w/ removing this and the override as well.

hub-cap · 2018-11-06T18:05:03Z

...h-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeSubmissionResponse.java

+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
+import org.elasticsearch.common.xcontent.ObjectParser;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.tasks.TaskId;


Im still leaning toward string. we have to have a way to deserialize bits from the wire into a TaskId, so there will be a way for a user to create their own TaskId, if they want to go that far.

…sts to fail

pgomulka · 2018-11-09T15:09:51Z

@hub-cap updated PR and build is passing too!

hub-cap

One nit and one question, other than that lgtm. Im approving but I would still like ot see the answer to the question about array vs single string.

hub-cap · 2018-11-12T14:18:59Z

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

@@ -49,13 +50,14 @@ static Request submitMigrateTask(IndexUpgradeRequest indexUpgradeRequest) {

    private static Request prepareMigrateRequest(IndexUpgradeRequest indexUpgradeRequest, boolean waitForCompletion) {
        RequestConverters.EndpointBuilder endpointBuilder = new RequestConverters.EndpointBuilder()
-            .addPathPartAsIs("_xpack/migration/upgrade")
-            .addCommaSeparatedPathParts(indexUpgradeRequest.indices());
+            .addPathPartAsIs("_xpack", "migration", "upgrade", indexUpgradeRequest.index());


This has to be

.addPathPartAsIs("Strings", "hard", "coded") .addPathPart(indexUpgradeRequest.index())

for sanitization

hub-cap · 2018-11-12T14:19:59Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/migration/IndexUpgradeRequest.java

-
-    private String[] indices;
-    private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, true, true, true);
+    private String index;


Why did we change this from an array of strings to a single one?

for other kind of requests you can specify indices comma separated like _xpack/migration/assistance/index1,index2,index3
but for upgrade only one is allowed (as per documentation)

Implement high level client for migration upgrade API. It should wrap RestHighLevelClient and expose high level IndexUpgradeRequest (new), IndexTaskResponse for submissions with wait_for_completion=false and BulkByScrollResponse (already used) objects. refers: elastic#29827

Implement high level client for migration upgrade API. It should wrap RestHighLevelClient and expose high level IndexUpgradeRequest (new), IndexTaskResponse for submissions with wait_for_completion=false and BulkByScrollResponse (already used) objects. refers: #29827 Original backported PR (#34898) PR of the backport (#35528)

pgomulka added the :Core/Java High Level REST Client label Oct 26, 2018

pgomulka requested a review from hub-cap October 26, 2018 12:03

elasticmachine mentioned this pull request Oct 26, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

pgomulka force-pushed the feature/hlrc-migration-api branch 3 times, most recently from a7f9368 to 826c19e Compare October 30, 2018 13:54

pgomulka commented Oct 30, 2018

View reviewed changes

pgomulka force-pushed the feature/hlrc-migration-api branch from ef85460 to a47e432 Compare October 30, 2018 16:27

pgomulka commented Oct 30, 2018

View reviewed changes

pgomulka added 2 commits November 2, 2018 07:54

fix exception handling

ff3ddd1

fix failing tests due to wait_for_completion changes

1af129c

hub-cap suggested changes Nov 2, 2018

View reviewed changes

pgomulka added 2 commits November 3, 2018 16:44

javadocs and cleanup

b6683d1

add response parsing test

bcb8f38

pgomulka commented Nov 5, 2018

View reviewed changes

hub-cap suggested changes Nov 5, 2018

View reviewed changes

review follow up

0deb3c1

hub-cap suggested changes Nov 6, 2018

View reviewed changes

pgomulka added 4 commits November 7, 2018 11:23

remove unused line

e63914f

Merge branch 'master' into feature/hlrc-migration-api

0e67a92

refactor returned type and request

a71d5c8

Merge branch 'master' into feature/hlrc-migration-api

0c14987

pgomulka added 2 commits November 8, 2018 18:36

remove index options and timeout as these are not used. causing reque…

30a1694

…sts to fail

Merge branch 'master' into feature/hlrc-migration-api

37fbf88

hub-cap approved these changes Nov 12, 2018

View reviewed changes

pgomulka added 2 commits November 13, 2018 08:12

Merge branch 'master' into feature/hlrc-migration-api

77875e4

cleanup migration request endpoint builder

614e711

pgomulka merged commit fd4cd80 into elastic:master Nov 13, 2018

pgomulka mentioned this pull request Nov 14, 2018

HLRC: remove duplication from ascii docs #35345

Closed

tomcallahan added v7.0.0 v6.6.0 labels Nov 16, 2018

tomcallahan added the >enhancement label Dec 18, 2018

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


		import static org.elasticsearch.test.AbstractXContentTestCase.xContentTester;

		public class IndexUpgradeSubmissionResponseTests extends ESTestCase {

		IndexUpgradeSubmissionResponse::fromXContent, Collections.emptySet());
		}

HLRC: migration api - upgrade #34898

HLRC: migration api - upgrade #34898

Conversation

pgomulka commented Oct 26, 2018 • edited Loading

elasticmachine commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Oct 30, 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

pgomulka commented Oct 30, 2018

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

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

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

Choose a reason for hiding this comment

pgomulka commented Nov 5, 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

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

pgomulka Nov 6, 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

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

pgomulka commented Nov 9, 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

pgomulka commented Oct 26, 2018 •

edited

Loading

pgomulka Oct 30, 2018 •

edited

Loading

pgomulka Nov 6, 2018 •

edited

Loading