Deprecate sorting in reindex #49458

henningandersen · 2019-11-21T18:54:14Z

Reindex sort never gave a guarantee about the order of documents being
indexed into the destination, though it could give a sense of locality
of source data.

It prevents us from doing resilient reindex and other optimizations and
it has therefore been deprecated.

Related to #47567

The deprecation notice in ILM documentations use of sorted reindex may need further refinement once we move to removing sort.

Reindex sort never gave a guarantee about the order of documents being indexed into the destination, though it could give a sense of locality of source data. It prevents us from doing resilient reindex and other optimizations and it has therefore been deprecated. Related to elastic#47567

elasticmachine · 2019-11-21T18:54:16Z

Pinging @elastic/es-distributed (:Distributed/Reindex)

Had to skip testing this prior to 7.6 in order to add the warning.

Tim-Brooks

LGTM

jrodewig · 2019-11-26T14:19:34Z

docs/reference/docs/reindex.asciidoc

 `sort`:::
 (Optional, list) A comma-separated list of `<field>:<direction>` pairs to sort by before indexing.
 Use in conjunction with `max_docs` to control what documents are reindexed.
+deprecated[7.6, sort in reindex is deprecated] *NOTE: Sorting in reindex is deprecated.*
+Sorting in reindex was never guaranteed to index
+documents in order and prevents future evolution of reindex such as resilience and performance
+improvements. If used in combination with `max_docs`, consider using a query filter instead.


I feel like the deprecated macro and the bold note are a bit redundant.

I'd place this content into a block deprecated macro like below.

`sort`::: + -- (Optional, list) A comma-separated list of `<field>:<direction>` pairs to sort by before indexing. Use in conjunction with `max_docs` to control what documents are reindexed. deprecated::[7.6,Sort in reindex is deprecated. Sorting in reindex was never guaranteed to index documents in order and prevents reindex such as resilience and performance improvements. If used in combination with `max_docs`, consider using a query filter instead.] --

I tried this now, but I get an error saying:

element listitem: validity error : Element listitem content does not follow the DTD

which is why I originally went for the simpler solution. My asciidoc skills are not super-advanced, so maybe there is a trick to make this work?

I was able to get this working on my end without that error.

Here's the commit on my fork if that helps:
https://github.com/jrodewig/elasticsearch/commit/7111829c9d08cba24af5dc776c53e958a7490a58

Feel free to cherry-pick it or copy/paste the contents.

There are a few tricks:

The delimiter setup is needed.

The deprecated text must be on a single line.

What you have is workable though. If you're not able to get the streamlined deprecated note working, I wouldn't hold up this PR. We can reformat it as a separate issue.

Thanks so much @jrodewig , worked out fine on one line (had to also escape the comma), see: aa2a7e0

jrodewig · 2019-11-26T14:20:31Z

docs/reference/docs/reindex.asciidoc

@@ -478,6 +478,10 @@ which defaults to a maximum size of 100 MB.
 `sort`:::
 (Optional, list) A comma-separated list of `<field>:<direction>` pairs to sort by before indexing.
 Use in conjunction with `max_docs` to control what documents are reindexed.
+deprecated[7.6, sort in reindex is deprecated] *NOTE: Sorting in reindex is deprecated.*
+Sorting in reindex was never guaranteed to index
+documents in order and prevents future evolution of reindex such as resilience and performance


I'd go with "further" rather than "future"

Suggested change

documents in order and prevents future evolution of reindex such as resilience and performance

documents in order and prevents further development of reindex, such as resilience and performance

jrodewig

Left a suggestion re: streamlining the deprecated note. Also left a minor editorial comment.

Otherwise LGTM.

jrodewig · 2019-11-26T14:35:09Z

docs/reference/docs/reindex.asciidoc

@@ -837,8 +818,8 @@ POST _reindex
 ----------------------------------------------------------------
 // TEST[setup:big_twitter]

-<1> `_reindex` defaults to sorting by `_doc` so `random_score` will not have any
-effect unless you override the sort to `_score`.
+<1> you may need to adjust the `min_score` depending on the relative amount of


Suggested change

<1> you may need to adjust the `min_score` depending on the relative amount of

<1> You may need to adjust the `min_score` depending on the relative amount of

jrodewig

LGTM. I can address reformatting the deprecated note as a separate issue.

Streamline deprecation notice and better text. Add deprecation notice to ILM reindex doc.

henningandersen · 2019-11-28T08:02:25Z

Thanks for reviewing @jrodewig and @tbrooks8.

henningandersen · 2019-11-28T12:37:35Z

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/packaging-sample-matrix

Reindex sort never gave a guarantee about the order of documents being indexed into the destination, though it could give a sense of locality of source data. It prevents us from doing resilient reindex and other optimizations and it has therefore been deprecated. Related to #47567

This reverts commit 27d45c9.

Reindex sort never gave a guarantee about the order of documents being indexed into the destination, though it could give a sense of locality of source data. It prevents us from doing resilient reindex and other optimizations and it has therefore been deprecated. Related to elastic#47567

Reindex sort never gave a guarantee about the order of documents being indexed into the destination, though it could give a sense of locality of source data. It prevents us from doing resilient reindex and other optimizations and it has therefore been deprecated. Related to #47567

Moved the deprecation warning to ReindexValidator to ensure it runs early and works with resilient reindex. Also check that the warning is reported back also for wait_for_completion=false. Follow-up to elastic#49458

Moved the deprecation warning to ReindexValidator to ensure it runs early and works with resilient reindex. Also check that the warning is reported back for wait_for_completion=false. Follow-up to #49458

Moved the deprecation warning to ReindexValidator to ensure it runs early and works with resilient reindex. Also check that the warning is reported back for wait_for_completion=false. Follow-up to elastic#49458

Moved the deprecation warning to ReindexValidator to ensure it runs early and works with resilient reindex. Also check that the warning is reported back for wait_for_completion=false. Follow-up to #49458

Reindex sort never gave a guarantee about the order of documents being indexed into the destination, though it could give a sense of locality of source data. It prevents us from doing resilient reindex and other optimizations and it has therefore been deprecated. Related to elastic#47567

Moved the deprecation warning to ReindexValidator to ensure it runs early and works with resilient reindex. Also check that the warning is reported back for wait_for_completion=false. Follow-up to elastic#49458

Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356

Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356 (cherry picked from commit 20a2133)

henningandersen added >deprecation v8.0.0 :Distributed Indexing/Reindex Issues relating to reindex that are not caused by issues further down v7.6.0 labels Nov 21, 2019

henningandersen added 3 commits November 22, 2019 08:35

Fixed test failure

dc8a875

Had to skip testing this prior to 7.6 in order to add the warning.

Improve documentation layout

d47eb08

Improve documentation layout

5c49c63

henningandersen requested a review from Tim-Brooks November 22, 2019 22:54

Merge remote-tracking branch 'origin/master' into deprecate_reindex_sort

459b66c

henningandersen force-pushed the deprecate_reindex_sort branch from e8d552a to 459b66c Compare November 22, 2019 23:02

Merge remote-tracking branch 'origin/master' into deprecate_reindex_sort

fcc5a73

Tim-Brooks approved these changes Nov 26, 2019

View reviewed changes

jrodewig reviewed Nov 26, 2019

View reviewed changes

jrodewig approved these changes Nov 27, 2019

View reviewed changes

Documentation fixes

aa2a7e0

Streamline deprecation notice and better text. Add deprecation notice to ILM reindex doc.

Merge remote-tracking branch 'origin/master' into deprecate_reindex_sort

6993834

henningandersen merged commit 5b56a99 into elastic:master Nov 29, 2019

henningandersen added a commit that referenced this pull request Nov 29, 2019

Revert "Deprecate sorting in reindex (#49458)"

1d745f1

This reverts commit 27d45c9.

henningandersen mentioned this pull request Dec 5, 2019

Reindex sort deprecation warning take 2 #49855

Merged

henningandersen mentioned this pull request Dec 6, 2019

Reindex sort deprecation warning take 2 (#49855) #49899

Merged

DaveCTurner mentioned this pull request Dec 20, 2019

GET _cat/nodes?local is not completely local #50088

Closed

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

Deprecate sort on ReindexOnServer elastic/elasticsearch-net#4356

Closed

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Feb 4, 2020

Deprecate sort on ReindexOnServer

481d18d

Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356

russcam mentioned this pull request Feb 4, 2020

Deprecate sort on ReindexOnServer elastic/elasticsearch-net#4357

Merged

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Feb 9, 2020

Deprecate sort on ReindexOnServer (#4357)

20a2133

Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate sorting in reindex #49458

Deprecate sorting in reindex #49458

henningandersen commented Nov 21, 2019 •

edited

Loading

elasticmachine commented Nov 21, 2019

Tim-Brooks left a comment

jrodewig Nov 26, 2019

henningandersen Nov 27, 2019

jrodewig Nov 27, 2019

jrodewig Nov 27, 2019

henningandersen Nov 28, 2019

jrodewig Nov 26, 2019

jrodewig left a comment •

edited

Loading

jrodewig Nov 26, 2019

jrodewig left a comment

henningandersen commented Nov 28, 2019

henningandersen commented Nov 28, 2019

	documents in order and prevents future evolution of reindex such as resilience and performance
	documents in order and prevents further development of reindex, such as resilience and performance

	<1> you may need to adjust the `min_score` depending on the relative amount of
	<1> You may need to adjust the `min_score` depending on the relative amount of

Deprecate sorting in reindex #49458

Deprecate sorting in reindex #49458

Conversation

henningandersen commented Nov 21, 2019 • edited Loading

elasticmachine commented Nov 21, 2019

Tim-Brooks left a comment

Choose a reason for hiding this comment

jrodewig Nov 26, 2019

Choose a reason for hiding this comment

henningandersen Nov 27, 2019

Choose a reason for hiding this comment

jrodewig Nov 27, 2019

Choose a reason for hiding this comment

jrodewig Nov 27, 2019

Choose a reason for hiding this comment

henningandersen Nov 28, 2019

Choose a reason for hiding this comment

jrodewig Nov 26, 2019

Choose a reason for hiding this comment

jrodewig left a comment • edited Loading

Choose a reason for hiding this comment

jrodewig Nov 26, 2019

Choose a reason for hiding this comment

jrodewig left a comment

Choose a reason for hiding this comment

henningandersen commented Nov 28, 2019

henningandersen commented Nov 28, 2019

henningandersen commented Nov 21, 2019 •

edited

Loading

jrodewig left a comment •

edited

Loading