-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Lower default mappings parsing compatibility to MINIMUM_READONLY_COMPATIBLE #119017
Lower default mappings parsing compatibility to MINIMUM_READONLY_COMPATIBLE #119017
Conversation
…ATIBLE Legacy indices check what minimum compatibility version the type parser supports, for every field type found in the mappings. Only some of the basic fields are supported, otherwise a placeholder mapper is created in place of the real field. The minimum supported version is v5 for the supported field mappers. For all the others, we can lower that from MINIMUM_COMPATIBLE to MINIMUM_READONLY_COMPATIBLE. This commit also centralizes the creation of type parsers that declare support for archive indices, as they all declare the same version.
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
||
public static TypeParser createTypeParserWithLegacySupport(BiFunction<String, MappingParserContext, Builder> builderFunction) { | ||
return new TypeParser(builderFunction, MINIMUM_LEGACY_COMPATIBILITY_VERSION); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the code a little easier to follow: there is one lower bound legacy compatibility constant, rather than one per mapper. After all the lower bound is always v5. It's also easier to follow what mappers provide legacy support, as they must now all call this method.
} | ||
|
||
public TypeParser( | ||
BiFunction<String, MappingParserContext, Builder> builderFunction, | ||
List<BiConsumer<String, MappingParserContext>> contextValidator | ||
) { | ||
this(builderFunction, (n, c) -> contextValidator.forEach(v -> v.accept(n, c)), IndexVersions.MINIMUM_COMPATIBLE); | ||
this(builderFunction, (n, c) -> contextValidator.forEach(v -> v.accept(n, c)), IndexVersions.MINIMUM_READONLY_COMPATIBLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effectively, lowering the minimum compatible doesn't take any effect, because we were checking the lower bound only for legacy indices which provide their specific lower bound. The default minimum compatible version could have been null for what it mattered.
} else { | ||
assert parser == null || parser.supportsVersion(indexVersionCreated); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this assertion, we are now checking the min compatible version for all indices, and making sure that things are consistent. That said, only legacy indices will ever check the version compatibility in prod code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, as far as I can see apart from lowering the default mininum version for all parsers to MINIMUM_READ_ONLY_COMPATIBLE this is only a refactoring. Correct me if I'm wrong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @javanna, nice refactoring. LGTM
@cbuescher correct, I left some comments in the PR to clarify the goal and the effect they have. |
Legacy indices check what minimum compatibility version the type parser supports, for every field type found in the mappings. Only some of the basic fields are supported, otherwise a placeholder mapper is created in place of the real field.
The minimum supported version is v5 for the supported field mappers. For all the others, we can lower that from MINIMUM_COMPATIBLE to MINIMUM_READONLY_COMPATIBLE.
This commit also centralizes the creation of type parsers that declare support for archive indices, as they all declare the same version.