Skip to content

Atomic mapping updates across types #22220

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

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Dec 16, 2016

This PR makes mapping updates atomic when multiple types in an index are updated. Mappings for an index are now applied in a single atomic operation, which also allows to optimize some of the cross-type updates and checks.

@ywelsch ywelsch added :Search Foundations/Mapping Index mappings, including merging and defining field types >enhancement v5.2.0 v6.0.0-alpha1 labels Dec 16, 2016
@ywelsch ywelsch requested a review from jpountz December 16, 2016 09:29
This commit makes mapping updates atomic when multiple types in an index are updated. Mappings for an index are now applied in a single go, which allows to
optimize some of the cross-type updates and checks.
@ywelsch ywelsch force-pushed the enhance/atomic-mapping-updates-across-types branch from 3d64d56 to b0bdcbf Compare December 19, 2016 09:41
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good to me. The one thing I noticed is that some parts of the code that used to raise MapperParsingException now throw an IAE instead. I don't think it is an issue, but I am pinging @bleskes about it since we had a discussion about standardizing those a while back and I'd like to make sure it won't break anything.

// only update entries if needed
updatedEntries = internalMerge(indexMetaData, MergeReason.MAPPING_RECOVERY, true, true);
} catch (Exception e) {
logger.warn((org.apache.logging.log4j.util.Supplier<?>) () -> new ParameterizedMessage("[{}] failed to apply mappings", index()), e);
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you using the supplier variant here?

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 want to log an exception as well as a parameterized message, so that's the only possible way?

@bleskes
Copy link
Contributor

bleskes commented Dec 19, 2016

The discussion around mapping exceptions we had (if I remember correctly) was around capturing mapping related issues with an incoming document during parsing. This was part of the effort to make failures a valid result of an indexing operation (rather than let exception bubble up to the user). Glancing at the PR, it seems it's all related to mapping merges, so I think we're good on that aspect.

@ywelsch ywelsch merged commit 63af03a into elastic:master Dec 19, 2016
@ywelsch
Copy link
Contributor Author

ywelsch commented Dec 19, 2016

Thanks @jpountz @bleskes

ywelsch added a commit that referenced this pull request Dec 19, 2016
This commit makes mapping updates atomic when multiple types in an index are updated. Mappings for an index are now applied in a single atomic operation, which also allows to optimize some of the cross-type updates and checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v5.2.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants