Deprecate sorting in reindex#49458
Merged
henningandersen merged 8 commits intoelastic:masterfrom Nov 29, 2019
Merged
Conversation
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
Collaborator
|
Pinging @elastic/es-distributed (:Distributed/Reindex) |
Had to skip testing this prior to 7.6 in order to add the warning.
e8d552a to
459b66c
Compare
jrodewig
reviewed
Nov 26, 2019
docs/reference/docs/reindex.asciidoc
Outdated
Comment on lines
+478
to
+484
| `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. |
Contributor
There was a problem hiding this comment.
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.]
--
Contributor
Author
There was a problem hiding this comment.
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?
Contributor
There was a problem hiding this comment.
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
deprecatedtext must be on a single line.
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
jrodewig
reviewed
Nov 26, 2019
docs/reference/docs/reindex.asciidoc
Outdated
| 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 |
Contributor
There was a problem hiding this comment.
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
reviewed
Nov 26, 2019
jrodewig
reviewed
Nov 26, 2019
docs/reference/docs/reindex.asciidoc
Outdated
|
|
||
| <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 |
Contributor
There was a problem hiding this comment.
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
approved these changes
Nov 27, 2019
Contributor
jrodewig
left a comment
There was a problem hiding this comment.
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.
Contributor
Author
|
Thanks for reviewing @jrodewig and @tbrooks8. |
Contributor
Author
|
@elasticmachine run elasticsearch-ci/bwc |
henningandersen
added a commit
that referenced
this pull request
Nov 29, 2019
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
henningandersen
added a commit
that referenced
this pull request
Nov 29, 2019
This reverts commit 27d45c9.
henningandersen
added a commit
to henningandersen/elasticsearch
that referenced
this pull request
Nov 29, 2019
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
henningandersen
added a commit
that referenced
this pull request
Dec 1, 2019
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
henningandersen
added a commit
to henningandersen/elasticsearch
that referenced
this pull request
Dec 5, 2019
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
henningandersen
added a commit
that referenced
this pull request
Dec 6, 2019
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
henningandersen
added a commit
to henningandersen/elasticsearch
that referenced
this pull request
Dec 6, 2019
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
henningandersen
added a commit
that referenced
this pull request
Dec 6, 2019
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
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
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
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
This was referenced Feb 3, 2020
russcam
added a commit
to elastic/elasticsearch-net
that referenced
this pull request
Feb 4, 2020
Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356
russcam
added a commit
to elastic/elasticsearch-net
that referenced
this pull request
Feb 9, 2020
Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356
russcam
added a commit
to elastic/elasticsearch-net
that referenced
this pull request
Feb 9, 2020
Relates: #4341 Deprecate sorting in reindex elastic/elasticsearch#49458 (issue: elastic/elasticsearch#47567) Closes #4356 (cherry picked from commit 20a2133)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.