HLRC: reindex API with wait_for_completion false #35202

pgomulka · 2018-11-02T12:00:52Z

Extend High Level Rest Client Reindex API to support requests with
wait_for_completion=false. This method will return a TaskID and results
can be queried with Task API

refers: #27205

elasticmachine · 2018-11-02T12:03:43Z

Pinging @elastic/es-core-infra

Extend High Level Rest Client Reindex API to support requests with wait_for_completion=false. This method will return a TaskID and results can be queried with Task API refers: elastic#27205

pgomulka · 2018-11-02T12:04:48Z

...est-high-level/src/main/java/org/elasticsearch/client/reindex/ReindexSubmissionResponse.java

+import java.io.IOException;
+import java.util.Objects;
+
+public class ReindexSubmissionResponse extends ActionResponse implements ToXContentObject {


this I hope to be refactored with a common TaskSubmissionResponse. This could be then used in migration api and other methods with wait_for_completion=false

markharwood · 2018-11-02T16:12:06Z

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

+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public final ReindexSubmissionResponse submitReindexTask(ReindexRequest reindexRequest, RequestOptions options) throws IOException {
+        return performRequestAndParseEntity(


I wonder if there's any value to returning a ReindexSubmissionResponse rather than just a TaskId here?

Yeah, maybe as TaskId is good here.

I was considering this, but the reason I stayed with a Wrapping object is that Rest API returns taskId wrapped inside an object where TaskID is just one field. Same is in DeleteJobResponse (ML).
However I don't mind breaking this convention. It would however look a bit odd inside TaskID to have a parser that treats taskId as a field within itself. See TaskID from latest revision where I did that.

Hmmmmmmm. I see. So the idea is that if we added more than as TaskId to the response then we could stick it on this object. We can do that in the REST response because of the way the JSON is structured. The way your have it we could do that here. I like that argument. Even though it looks like an extra argument, it makes sense. It might be worth sticking on the Javadoc of that object. Somewhere.

Im +1 for a response object matching the response name (like ReindexSubmissionResponse) for this "adding things to the API" reason as well.

I was thinking of staying with TaskSubmissionResponse with String taskId field. As this would fit into other wait_for_completion=false type methods as well.

We could in theory always make the taskSubmission abstract and return concrete responses - with all the parsing and fields being already in the abstract class. That would be a bit too complex for that I guess

I'm fine with TaskSubmissionResponse. And I'm fine with String taskId. I'd also be fine with a client-side TaskId class that is super opaque. It is an identifier that client's aren't meant to pick apart. A String is fine for that though.

nik9000 · 2018-11-02T18:01:46Z

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

+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public final ReindexSubmissionResponse submitReindexTask(ReindexRequest reindexRequest, RequestOptions options) throws IOException {
+        return performRequestAndParseEntity(


Yeah, maybe as TaskId is good here.

nik9000 · 2018-11-02T18:04:08Z

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

+
+public class ReindexIT extends ESRestHighLevelClientTestCase {
+
+    //TODO taken from CrudIT - should these scenarios be moved here?


I'm fine with moving them. Especially if it lets us save some copy and pasting.

pgomulka · 2018-11-05T09:01:11Z

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

+     * @return the submission response
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public final TaskId submitReindexTask(ReindexRequest reindexRequest, RequestOptions options) throws IOException {


The name submit is breaking the API spec contract. We have test that verifies if we are not adding any additional methods apart from those from the API spec.

RestHighLevelClientTests.testApiNamingConventions <<< FAILURES! 19:16:19 > Throwable #1: java.lang.AssertionError: Some client method doesn't match a corresponding API defined in the REST spec: [submit_reindex_task] 19:16:19 > Expected: <0> 19:16:19 > but: was <1>
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/1777/console

Yeah I just hit that one too trying to return a GetTaskResponse wrapped in an Optional (following discussion on how Java clents should handle 404s)

I think maybe you can teach it about these sorts of names. They aren't required for every method, but maybe we could make them required if wait_for_completion was a parameter in the spec.

This reverts commit 686f10a.

make rest client test assert about submit task methods

pgomulka · 2018-11-06T09:49:39Z

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

@@ -107,6 +107,9 @@

 public class RestHighLevelClientTests extends ESTestCase {


we don't run this test for subclients like tasks() migraiton() etc
maybe we should..

Oh! We almost certainly should. But we'd have to do some "interesting" work to get the specs in place. We probably could come up with something though.

hub-cap

Great work here, love the test checks. We can merge this or you can update the TaskId stuff, assuming we agree on a type soon :P

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

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

+            );
+        }
+        {
+            // test1: create one doc in dest


what does this comment mean? i see it in a few places in this file

I will rename this - I guess someone meant to reindex one document with Id 1 from source to destination

hub-cap · 2018-11-06T18:46:54Z

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

+        assertThat("the second parameter to submit task method [" + method + "] is the wrong type",
+            method.getParameterTypes()[1], equalTo(RequestOptions.class));
+
+        assertThat("submit task method [" + method + "] must have wait_for_completion parameter in rest spec",


* elastic/master: (25 commits) Fixes fast vector highlighter docs per issue 24318. (elastic#34190) [ML] Prevent notifications on deletion of a non existent job (elastic#35337) [CCR] Auto follow Coordinator fetch cluster state in system context (elastic#35120) Mute test for elastic#35361 Preserve `date_histogram` format when aggregating on unmapped fields (elastic#35254) Test: Mute failing SSL test Allow unmapped fields in composite aggregations (elastic#35331) [RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) (elastic#34902) HLRC: reindex API with wait_for_completion false (elastic#35202) Add docs on JNA temp directory not being noexec (elastic#35355) [CCR] Adjust list of dynamic index settings that should be replicated (elastic#35195) Replicate index settings to followers (elastic#35089) Rename RealmConfig.globalSettings() to settings() (elastic#35330) [TEST] Cleanup FileUserPasswdStoreTests (elastic#35329) Scripting: Add back lookup vars in score script (elastic#34833) watcher: Fix integration tests to ensure correct start/stop of Watcher (elastic#35271) Remove ALL shard check in CheckShrinkReadyStep (elastic#35346) Use soft-deleted docs to resolve strategy for engine operation (elastic#35230) [ILM] Check shard and relocation status in AllocationRoutedStep (elastic#35316) Ignore date ranges containing 'now' when pre-processing a percolator query (elastic#35160) ...

pcsanwald · 2018-11-09T12:27:20Z

@pgomulka I added v7.0.0 label here, please remove if that seems incorrect.

Extend High Level Rest Client Reindex API to support requests with wait_for_completion=false. This method will return a TaskSubmissionResult with task identifier as string and results can be queried with Task API refers: elastic#27205

javanna · 2018-11-13T15:00:39Z

@pgomulka I think this needs backporting, the 6.6.0 label was added but I don't see these changes in the 6.x branch.

pgomulka · 2018-11-14T07:19:11Z

@javanna sorry about that. Are all HLRC changes supposed to be in 6.x as well? The HLRC is labeled 7.0 in clubhouse

Extend High Level Rest Client Reindex API to support requests with wait_for_completion=false. This method will return a TaskSubmissionResult with task identifier as string and results can be queried with Task API refers: elastic#27205

Extend High Level Rest Client Reindex API to support requests with wait_for_completion=false. This method will return a TaskSubmissionResult with task identifier as string and results can be queried with Task API refers: #27205 Original PR against master #35202 Back Port PR #35527

pgomulka force-pushed the feature/reindex-wait-for-completion branch from 33c7e99 to 0195e7b Compare November 2, 2018 12:02

pgomulka requested review from hub-cap and markharwood November 2, 2018 12:03

pgomulka added the :Core/Java High Level REST Client label Nov 2, 2018

HLRC: reindex API with wait_for_completion false

ec3c93d

Extend High Level Rest Client Reindex API to support requests with wait_for_completion=false. This method will return a TaskID and results can be queried with Task API refers: elastic#27205

pgomulka force-pushed the feature/reindex-wait-for-completion branch from 0195e7b to ec3c93d Compare November 2, 2018 13:31

pgomulka commented Nov 2, 2018

View reviewed changes

markharwood reviewed Nov 2, 2018

View reviewed changes

nik9000 reviewed Nov 2, 2018

View reviewed changes

pgomulka added 3 commits November 3, 2018 18:35

changes due ot wait for completion flag

3cc288d

cleanup docs

0aa1294

refactor to return taskID from submission

686f10a

pgomulka commented Nov 5, 2018

View reviewed changes

pgomulka added 2 commits November 6, 2018 09:10

Revert "refactor to return taskID from submission"

16f2550

This reverts commit 686f10a.

return TaskSubmissionResponse

d3ca238

make rest client test assert about submit task methods

pgomulka commented Nov 6, 2018

View reviewed changes

pgomulka added 3 commits November 6, 2018 12:18

missing headers

ef40220

Merge branch 'master' into feature/reindex-wait-for-completion

ebf0f53

rest high level client tests after merge

5df4cbb

hub-cap approved these changes Nov 6, 2018

View reviewed changes

pgomulka added 2 commits November 7, 2018 10:51

refactor taskid to be a string

f66f7b0

Merge branch 'master' into feature/reindex-wait-for-completion

acc66f6

pgomulka merged commit a90ef6b into elastic:master Nov 8, 2018

pcsanwald added the v7.0.0 label Nov 9, 2018

hub-cap added v6.6.0 >enhancement labels Nov 12, 2018

javanna mentioned this pull request Nov 13, 2018

HLRC: add support for get license basic/trial status API #33176

Merged

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

jbonn360 mentioned this pull request Sep 13, 2019

shouldStoreResult is not set correctly #46350

Closed

tumile mentioned this pull request Jun 25, 2020

HLRC: UpdateByQuery API with wait_for_completion being false #58552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLRC: reindex API with wait_for_completion false #35202

HLRC: reindex API with wait_for_completion false #35202

pgomulka commented Nov 2, 2018

elasticmachine commented Nov 2, 2018

pgomulka Nov 2, 2018

markharwood Nov 2, 2018

nik9000 Nov 2, 2018

pgomulka Nov 5, 2018

nik9000 Nov 5, 2018

hub-cap Nov 6, 2018

pgomulka Nov 7, 2018

nik9000 Nov 7, 2018

nik9000 Nov 2, 2018

nik9000 Nov 2, 2018

pgomulka Nov 5, 2018

markharwood Nov 5, 2018

nik9000 Nov 5, 2018

pgomulka Nov 6, 2018

nik9000 Nov 7, 2018

hub-cap left a comment

hub-cap Nov 6, 2018

pgomulka Nov 7, 2018

hub-cap Nov 6, 2018

pcsanwald commented Nov 9, 2018

javanna commented Nov 13, 2018

pgomulka commented Nov 14, 2018 •

edited

Loading


		public class ReindexIT extends ESRestHighLevelClientTestCase {

		//TODO taken from CrudIT - should these scenarios be moved here?

		@@ -107,6 +107,9 @@

		public class RestHighLevelClientTests extends ESTestCase {

HLRC: reindex API with wait_for_completion false #35202

HLRC: reindex API with wait_for_completion false #35202

Conversation

pgomulka commented Nov 2, 2018

elasticmachine commented Nov 2, 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

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

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

pcsanwald commented Nov 9, 2018

javanna commented Nov 13, 2018

pgomulka commented Nov 14, 2018 • edited Loading

pgomulka commented Nov 14, 2018 •

edited

Loading