Add scroll parameter to _reindex API by PnPie · Pull Request #28041 · elastic/elasticsearch

PnPie · 2018-01-01T21:42:43Z

Be able to change scroll timeout in _reindex API (by default: 5m)
Close #27555

Be able to change scroll timeout in _reindex API (by default: 5m)

elasticmachine · 2018-01-01T21:42:44Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-01-01T21:42:44Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jimczi

Thanks @PnPie
I left one comment, I think this new option should be applicable to all bulk by scroll actions.

jimczi · 2018-01-04T09:35:55Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java

        try (XContentParser parser = request.contentParser()) {
            PARSER.parse(parser, internal, null);
        }
+        if (request.hasParam("scroll")) {


Since you changed AbstractBulkByScrollRequest to accept a scroll parameter you could parse this option directly in AbstractBaseReindexRestHandler#setCommonOptions. This way all reindex actions would be able to set this option (_delete_by_query, _update_by_query`).

Thx for your quick review. In fact I see that for _delete_by_query and _update_by_query, we have already parsed the rest request here in their superclass https://github.com/elastic/elasticsearch/blob/master/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java#L54 by calling RestSearchAction#parseSearchRequest, so also including scroll parameter. I'm not sure for RestReindexAction we should do the same thing or not (as _delete_by_query and _update_by_query have a superclass of their own) ? So I just added it in RestReindexAction.

You're right it's not needed for _delete_by_query and _update_by_query since they parse the request with RestSearchAction. I missed this part so I think that this pr is ready and fixes the main issue; adding the support to change the default scroll for _reindex request.

jimczi

LGTM, thanks @PnPie, I'll merge soon.

jimczi · 2018-01-05T08:38:00Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java

        try (XContentParser parser = request.contentParser()) {
            PARSER.parse(parser, internal, null);
        }
+        if (request.hasParam("scroll")) {


You're right it's not needed for _delete_by_query and _update_by_query since they parse the request with RestSearchAction. I missed this part so I think that this pr is ready and fixes the main issue; adding the support to change the default scroll for _reindex request.

jimczi · 2018-01-05T09:13:39Z

@elasticmachine OK to test

jimczi · 2018-01-05T14:27:07Z

@elasticmachine test this please

nik9000 · 2018-01-09T14:38:45Z

This makes sense to me. Is it worth documenting that you can do this in reindex.asciidoc, update_by_query.asciidoc, and delete_by_query.asciidoc? I don't expect people to change this all that much but documenting it might give us a better story about how it is used.

update docs for reindex, delete by query, and update by query

PnPie · 2018-01-09T20:04:54Z

Of course @nik9000, I added docs for these 3 :)

nik9000

Thanks!

PnPie · 2018-01-10T19:02:27Z

Hello,
In another PR I made, @rjernst told me that we may need to merge master into the PR because we just started using gradle wrapper. I also see the build failed because of this kind of problem. So shall I need to do a merge from master ?

jimczi · 2018-01-10T19:35:47Z

Yes please, this will make this pr testable by our CI, otherwise I'd add to run the tests on my machine before merging this pr.

…tscroll_to_reindex_api

jimczi · 2018-01-11T08:55:08Z

@elasticmachine test this please

Be able to change scroll timeout in _reindex API (by default: 5m)

jimczi · 2018-01-11T14:05:59Z

Thanks @PnPie !

* master: (43 commits) Rename core module to server (#28180) upgraded jna from 4.4.0-1 to 4.5.1 (#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (#28041) Include all sentences smaller than fragment_size in the unified highlighter (#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (#28019) Ignore null value for range field (#27845) (#28116) Fix environment variable substitutions in list setting (#28106) docs: Replaces indexed script java api docs with stored script api docs test: ensure we endup with a single segment Make sure that we don't detect files as maven coordinate when installing a plugin (#28163) [Tests] temporary disable meta plugin rest tests #28163 meta-plugin should install bin and config at the top level (#28162) Painless: Add public member read/write access test. (#28156) Docs: Clarify password protection support with keystore (#28157) [Docs] fix plugin properties inclusion for plugins authors ...

* 6.x: (41 commits) Rename core module to server (#28190) [Test] Remove superfluous lines [Test] Add skip test for index range field with null values upgraded jna from 4.4.0-1 to 4.5.1 (#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads [Docs] Correct response json in rank-eval.asciidoc Fix environment variable substitutions in list setting (#28106) Add scroll parameter to _reindex API (#28041) Include all sentences smaller than fragment_size in the unified highlighter (#28132) [Docs] Improvements in script-fields.asciidoc (#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (#28019) Ignore null value for range field (#27845) (#28116) docs: Replaces indexed script java api docs with stored script api docs test: ensure we endup with a single segment Make sure that we don't detect files as maven coordinate when installing a plugin (#28163) [Tests] temporary disable meta plugin rest tests #28163 meta-plugin should install bin and config at the top level (#28162) Painless: Add public member read/write access test. (#28156) Docs: Clarify password protection support with keystore (#28157) [Docs] fix plugin properties inclusion for plugins authors ...

* master: (30 commits) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (elastic#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (elastic#28041) Include all sentences smaller than fragment_size in the unified highlighter (elastic#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (elastic#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (elastic#28019) Ignore null value for range field (elastic#27845) (elastic#28116) Fix environment variable substitutions in list setting (elastic#28106) ...

* compile-with-jdk-9: (56 commits) TEST: init unassigned gcp in testAcquireIndexCommit Replica start peer recovery with safe commit (elastic#28181) Truncate tlog cli should assign global checkpoint (elastic#28192) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (elastic#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (elastic#28041) Include all sentences smaller than fragment_size in the unified highlighter (elastic#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (elastic#28174) ...

Add scroll parameter to _reindex API

6910d88

Be able to change scroll timeout in _reindex API (by default: 5m)

jimczi requested changes Jan 4, 2018

View reviewed changes

mayya-sharipova added >feature :Reindex API labels Jan 4, 2018

jimczi approved these changes Jan 5, 2018

View reviewed changes

jimczi added v6.2.0 v7.0.0 labels Jan 5, 2018

Add docs for scroll parameter

fdfe418

update docs for reindex, delete by query, and update by query

nik9000 approved these changes Jan 9, 2018

View reviewed changes

Merge branch 'master' of github.com:elastic/elasticsearch into add_se…

0cd71e5

…tscroll_to_reindex_api

jimczi merged commit 228f7ff into elastic:master Jan 11, 2018

jimczi pushed a commit that referenced this pull request Jan 11, 2018

Add scroll parameter to _reindex API (#28041)

f136798

Be able to change scroll timeout in _reindex API (by default: 5m)

lcawl added :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. and removed :Reindex API labels Feb 13, 2018

codebrain mentioned this pull request May 23, 2018

[6.2] Add scroll parameter to _reindex API elastic/elasticsearch-net#3253

Closed

codebrain added a commit to codebrain/elasticsearch that referenced this pull request Jun 14, 2018

Add scroll paramter to reindex JSON spec as per elastic#28041

30f349c

codebrain mentioned this pull request Jun 14, 2018

Add scroll parameter to reindex JSON spec as per #28041 #31316

Open

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

Conversation

PnPie commented Jan 1, 2018

Uh oh!

elasticmachine commented Jan 1, 2018

Uh oh!

elasticmachine commented Jan 1, 2018

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

PnPie Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

jimczi Jan 5, 2018

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi Jan 5, 2018

Choose a reason for hiding this comment

Uh oh!

jimczi commented Jan 5, 2018

Uh oh!

jimczi commented Jan 5, 2018

Uh oh!

nik9000 commented Jan 9, 2018

Uh oh!

PnPie commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

PnPie commented Jan 10, 2018

Uh oh!

jimczi commented Jan 10, 2018

Uh oh!

jimczi commented Jan 11, 2018

Uh oh!

jimczi commented Jan 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PnPie commented Jan 9, 2018 •

edited

Loading