Skip to content

Reindex source types disregarded in 7.x #49580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 15, 2019

Conversation

henningandersen
Copy link
Contributor

Clarify that types in source index are disregarded.

Closes #48460

Clarify that types in source index are disregarded.

Closes elastic#48460
@henningandersen henningandersen added :Distributed Indexing/Reindex Issues relating to reindex that are not caused by issues further down >docs General docs changes v7.5.1 v7.6.0 labels Nov 26, 2019
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

Comment on lines 507 to 510
NOTE: Types in source indices are always ignored, also when not specifying a
destination `type`. If explicitly specifying destination `type`, the specified
type must match the type in the destination index or be either unspecified or
the special value `_doc`. See <<removal-of-types>> for further details.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are two problems here:

  1. The type parameters aren't documented.
  2. It's not clear that this is chiefly for users trying to preserve their existing document types.

I pushed f669009 and a8087be to this PR to address both of those.

Feel free to change or revert this commits if wanted. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree to 1, moving it to a definition of the type parameter makes sense.

About 2, while they might be trying to preserve their types, they were in reality just expecting an error in this case and I wanted to clarify that the source type is disregarded completely now. In older versions, ES would copy over the source type to the destination too, but now that dest have only one type (which in 7.x can still be freely specified) copying over the type makes little to no sense. The source could in principle still have multiple types if specifying multiple indices with different types.

I commented on this inline too.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @henningandersen.

I think the first commit was a bit unclear so I pushed f669009 and a8087be to try to address.

Feel free to revert or adjust those as you see fit.

@csenol
Copy link

csenol commented Dec 10, 2019

user feedback: Thanks for this. This could have helped us.
I hope users in the future will be aware of this with the given Warning

@@ -484,6 +484,18 @@ Use in conjunction with `max_docs` to control what documents are reindexed.
Set to a list to reindex select fields.
Defaults to `true`.

`type`:::
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should leave out this part of the change, since it solely concerns the source type and this is just a filter on the source. I think adding it here increases the risk of it being used, so would kind of prefer not to add this now.

It is also unrelated to the issue, since regardless of specifying this filter, the type from source is disregarded when indexing into dest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with removing this. As you mentioned, this isn't the main problem.

+
[WARNING]
====
By default, all reindexed documents are assigned the `_doc` document type. To
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original problem here really was that the user expected an error. I think that is somewhat lost in this new version and I prefer the original text (here) instead. WDYT?

Suggested change
By default, all reindexed documents are assigned the `_doc` document type. To
Types in source indices are always ignored, also when not specifying a
destination `type`. If explicitly specifying destination `type`, the specified
type must match the type in the destination index or be either unspecified or
the special value `_doc`. See <<removal-of-types>> for further details.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 That works for me.

Comment on lines 507 to 510
NOTE: Types in source indices are always ignored, also when not specifying a
destination `type`. If explicitly specifying destination `type`, the specified
type must match the type in the destination index or be either unspecified or
the special value `_doc`. See <<removal-of-types>> for further details.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree to 1, moving it to a definition of the type parameter makes sense.

About 2, while they might be trying to preserve their types, they were in reality just expecting an error in this case and I wanted to clarify that the source type is disregarded completely now. In older versions, ES would copy over the source type to the destination too, but now that dest have only one type (which in 7.x can still be freely specified) copying over the type makes little to no sense. The source could in principle still have multiple types if specifying multiple indices with different types.

I commented on this inline too.

Copy link
Contributor Author

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @jrodewig , I have a few comments to your changes. I do not mind carrying out the final changes, but thought we could agree on them here first.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks @henningandersen. I agree with your suggestions. Thanks for seeing this one through.

Remove source type description and clarify that source type is
disregarded completely.
Copy link
Contributor

@jrodewig jrodewig 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 again for the iteration @henningandersen.

@jpountz jpountz added v7.5.2 and removed v7.5.1 labels Dec 13, 2019
@henningandersen
Copy link
Contributor Author

Thanks for reviewing @jrodewig

@henningandersen henningandersen merged commit a0044df into elastic:7.x Dec 15, 2019
henningandersen added a commit that referenced this pull request Dec 15, 2019
Clarify that types in source index are disregarded.
@jasontedor jasontedor added v7.5.1 and removed v7.5.2 labels Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Reindex Issues relating to reindex that are not caused by issues further down >docs General docs changes v7.5.1 v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants