Skip to content

HLRC: Add support for reindex rethrottling#33832

Merged
cbuescher merged 2 commits intoelastic:masterfrom
cbuescher:hl-client-rethrottle
Sep 20, 2018
Merged

HLRC: Add support for reindex rethrottling#33832
cbuescher merged 2 commits intoelastic:masterfrom
cbuescher:hl-client-rethrottle

Conversation

@cbuescher
Copy link
Copy Markdown
Member

@cbuescher cbuescher commented Sep 18, 2018

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

@cbuescher cbuescher requested a review from nik9000 September 18, 2018 17:30
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 18, 2018

Pinging @elastic/es-core-infra

cbuescher
cbuescher commented Sep 18, 2018
View reviewed changes
client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java Outdated
Copy link
Copy Markdown
Member Author

@cbuescher cbuescher Sep 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@cbuescher cbuescher Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member Author

cbuescher commented Sep 19, 2018

@elasticmachine run gradle build tests

nik9000
nik9000 requested changes Sep 19, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java Outdated
Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@cbuescher cbuescher Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@cbuescher cbuescher Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this in a commit there.

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java Outdated
Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@cbuescher cbuescher Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java Outdated
Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java Outdated
Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent these a bit more please!

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java Outdated
Copy link
Copy Markdown
Member

@nik9000 nik9000 Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you name it bulkRequest or something like that?

@cbuescher
Copy link
Copy Markdown
Member Author

cbuescher commented Sep 19, 2018

@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.

@cbuescher cbuescher mentioned this pull request Sep 19, 2018
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
Copy link
Copy Markdown
Member Author

cbuescher commented Sep 20, 2018

@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
nik9000 approved these changes Sep 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@cbuescher 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 cbuescher mentioned this pull request Sep 21, 2018
Merged
kcm pushed a commit that referenced this pull request Oct 30, 2018
@kcm
HLRC: Add support for reindex rethrottling (#33832)
74cbe0c 
This change adds support for rethrottling reindex requests to the
RestHighLevelClient.
@cbuescher cbuescher deleted the hl-client-rethrottle branch November 27, 2025 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees

No one assigned

Labels

>enhancement v6.5.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cbuescher @elasticmachine @nik9000 @colings86