Skip to content

Fix reindex with a remote source on a version before 2.0.0#23805

Merged
jimczi merged 4 commits intoelastic:masterfrom
jimczi:reindex_from_remote_1.7
Mar 31, 2017
Merged

Fix reindex with a remote source on a version before 2.0.0#23805
jimczi merged 4 commits intoelastic:masterfrom
jimczi:reindex_from_remote_1.7

Conversation

@jimczi
Copy link
Copy Markdown
Contributor

@jimczi jimczi commented Mar 29, 2017

Discussed in https://discuss.elastic.co/t/5-3-0-breaks--reindex/80388 and #22931 (comment)

This commit 7520a10#diff-0c925d1f41390dd5d7dd09b6ff5d51d1 in master and v5.3 breaks reindex from remote when the source is on a version before 2.0.0. The scroll_id is always extracted from a plain text body in ES 1.7 (and before) and does not accept the JSON version that appears in 2.0.0.
This change checks the version of the remote cluster and builds the scroll_id in the body depending on the remote version.

Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM. I think this can safely go into a 5.3 patch release?

@jimczi jimczi added the v5.3.1 label Mar 29, 2017
Copy link
Copy Markdown
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

This is such an important part of our upgrade strategy that i think we need a real integration test for it as well

@jimczi
Copy link
Copy Markdown
Contributor Author

jimczi commented Mar 30, 2017

@bleskes it would be great I agree but it's not low hanging fruit to build an IT that launches a node in 1.7 or maybe I missed something ?

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Mar 30, 2017

@bleskes it would be great I agree but it's not low hanging fruit to build an IT that launches a node in 1.7 or maybe I missed something ?

Right. We don't have the infrastructure built to start a 1.7 version of Elasticsearch in the integration test suite. I'd certainly love to have it though.

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.

This fixes the issues, yeah.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Mar 30, 2017

I filed #23828 to have a look at reindex-from-remote.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Mar 30, 2017

I filed #23828 to have a look at reindex-from-remote.

Sorry, to have a look at testing reindex-from-remote more completely.

@bleskes
Copy link
Copy Markdown
Contributor

bleskes commented Mar 30, 2017 via email

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Mar 30, 2017

Thank you Nik. To be clear I by no means meant that the suggested test is a requirement to get this in.

I figured as much. I'm really sorry I didn't write those tests after I wrote reindex-from-remote in the first place. It would have saved so much trouble.

@jimczi jimczi merged commit f3a925f into elastic:master Mar 31, 2017
@jimczi jimczi deleted the reindex_from_remote_1.7 branch March 31, 2017 07:07
jimczi added a commit that referenced this pull request Mar 31, 2017
Send the scroll id in the body as plain text when the remote version is before 2.0.0
jimczi added a commit that referenced this pull request Mar 31, 2017
Send the scroll id in the body as plain text when the remote version is before 2.0.0
@lcawl 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v5.3.1 v5.4.0 v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants