HLRC: Add support for reindex rethrottling #33832

cbuescher · 2018-09-18T17:30:57Z

This change adds support for rethrottling reindex requests to the
RestHighLevelClient.

elasticmachine · 2018-09-18T17:30:58Z

Pinging @elastic/es-core-infra

cbuescher · 2018-09-18T17:36:16Z

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

+    public final ListTasksResponse reindexRethrottle(RethrottleRequest rethrottleRequest, RequestOptions options) throws IOException {
+        return performRequestAndParseEntity(rethrottleRequest, RequestConverters::rethrottle, options, ListTasksResponse::fromXContent,
+                emptySet());
+    }


I wasn't sure if I should also include the analogous deleteByQueryRethrottle and updateByQueryRethrottle methods in this PR since I'm not sure how we want to expose the three rethrottling types. Maybe we just want a single method since I thin the Request/Response objects are mostly identical? They even target the same RestAction I think. On the other hand, if at any point in the future we want to untie those, maybe it makes sense to add separate method pairs for each flavour, wdyt?

I think we said we'd like to line the client up with the REST API. So, yes, I'd make all three of them.

Okay, thanks. I'd probably prefer to that in a follow-up PR though then, its a bit of a copy/paste job but then we also need to add two more documentation cases.

cbuescher · 2018-09-19T12:48:24Z

@elasticmachine run gradle build tests

nik9000

I left a few minor things. Sorry I didn't get to this yesterday!

nik9000 · 2018-09-19T13:37:39Z

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

@@ -465,7 +465,8 @@ static Request reindex(ReindexRequest reindexRequest) throws IOException {
        Params params = new Params(request)
            .withRefresh(reindexRequest.isRefresh())
            .withTimeout(reindexRequest.getTimeout())
-            .withWaitForActiveShards(reindexRequest.getWaitForActiveShards());
+            .withWaitForActiveShards(reindexRequest.getWaitForActiveShards())
+            .withRequestsPerSecond(reindexRequest.getRequestsPerSecond());


hmmmm - I think maybe this should be fixed on update by query and delete by query too.

I opened a separate PR for this change (#33808), can you review that one first and we can add this change there already? Its a bit unrelated to adding the rethrottle API so I factored it out.

I added this in a commit there.

nik9000 · 2018-09-19T13:38:49Z

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

+        Params withRequestsPerSecond(float requestsPerSecond) {
+            // the default in AbstractBulkByScrollRequest is Float.POSITIVE_INFINITY,
+            // but we don't want to add that to the URL parameters, instead we use -1
+            if (Float.isFinite(requestsPerSecond)) {


I don't think we need to add it to the url at all if it is POSITIVE_INFINITY.

We seem to need to add something, because I got errors from RestRethrottleAction#prepareRequest if not setting the parameter at all. AbstractBaseReindexRestHandler.parseRequestsPerSecond seems to be happy with -1 though.

I think it only needs to add -1 to the rethrottle API. Reindex doesn't need it if it is the default. But I suppose it doesn't hurt to pass it. I withdraw my comment.

nik9000 · 2018-09-19T13:40:04Z

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

+    public final ListTasksResponse reindexRethrottle(RethrottleRequest rethrottleRequest, RequestOptions options) throws IOException {
+        return performRequestAndParseEntity(rethrottleRequest, RequestConverters::rethrottle, options, ListTasksResponse::fromXContent,
+                emptySet());
+    }


I think we said we'd like to line the client up with the REST API. So, yes, I'd make all three of them.

nik9000 · 2018-09-19T13:40:44Z

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

@@ -642,15 +655,16 @@ public void testReindex() throws IOException {
                .build();
            createIndex(sourceIndex, settings);
            createIndex(destinationIndex, settings);
+            BulkRequest setRefreshPolicy = new BulkRequest()
+            .add(new IndexRequest(sourceIndex, "type", "1")


Indent these a bit more please!

nik9000 · 2018-09-19T13:41:36Z

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

@@ -642,15 +655,16 @@ public void testReindex() throws IOException {
                .build();
            createIndex(sourceIndex, settings);
            createIndex(destinationIndex, settings);
+            BulkRequest setRefreshPolicy = new BulkRequest()


Could you name it bulkRequest or something like that?

cbuescher · 2018-09-19T16:21:53Z

@nik9000 thanks for the review, I adressed some comments and will add a commit to #33808. Could you review that one first, this PR is somewhat based on that one.

This change adds support for rethrottling reindex requests to the RestHighLevelClient.

cbuescher · 2018-09-20T13:39:00Z

@nik9000 I merged #33808 and rebased on master, is there anything left to do on this PR? I would open another PR for the deleteByQuery and updateByQuery throttling.

nik9000

LGTM. Thanks!

This change adds support for rethrottling reindex requests to the RestHighLevelClient.

cbuescher added >enhancement v7.0.0 :Core/Java High Level REST Client v6.5.0 labels Sep 18, 2018

cbuescher requested a review from nik9000 September 18, 2018 17:30

cbuescher commented Sep 18, 2018

View reviewed changes

cbuescher force-pushed the hl-client-rethrottle branch from 2893531 to dc51932 Compare September 19, 2018 09:08

nik9000 requested changes Sep 19, 2018

View reviewed changes

cbuescher mentioned this pull request Sep 19, 2018

HLRC: Reindex should support requests_per_seconds parameter #33808

Merged

Christoph Büscher added 2 commits September 20, 2018 15:37

HLRC: Add support for reindex rethrottling

88fbe41

This change adds support for rethrottling reindex requests to the RestHighLevelClient.

Adressing comments

de9ee4f

cbuescher force-pushed the hl-client-rethrottle branch from dfb61c0 to de9ee4f Compare September 20, 2018 13:37

nik9000 approved these changes Sep 20, 2018

View reviewed changes

cbuescher merged commit 77145bb into elastic:master Sep 20, 2018

cbuescher pushed a commit that referenced this pull request Sep 20, 2018

HLRC: Add support for reindex rethrottling (#33832)

b253f36

This change adds support for rethrottling reindex requests to the RestHighLevelClient.

cbuescher mentioned this pull request Sep 21, 2018

HLRC: Add throttling for update & delete-by-query #33951

Merged

kcm pushed a commit that referenced this pull request Oct 30, 2018

HLRC: Add support for reindex rethrottling (#33832)

74cbe0c

This change adds support for rethrottling reindex requests to the RestHighLevelClient.

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

HLRC: Add support for reindex rethrottling #33832

HLRC: Add support for reindex rethrottling #33832

Uh oh!

Conversation

cbuescher commented Sep 18, 2018

Uh oh!

elasticmachine commented Sep 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!

cbuescher commented Sep 19, 2018

Uh oh!

nik9000 left a comment

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!

cbuescher commented Sep 19, 2018

Uh oh!

cbuescher commented Sep 20, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!