Skip to content

Adding a deprecation info api check for custom types in templates #80676

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 16, 2021
Merged

Adding a deprecation info api check for custom types in templates #80676

merged 4 commits into from
Nov 16, 2021

Conversation

masseyke
Copy link
Member

This commit adds a deprecation info API check for custom types in mappings in legacy templates.

@masseyke masseyke requested a review from jbaiera November 11, 2021 20:28
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Nov 11, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@jakelandis
Copy link
Contributor

jakelandis commented Nov 11, 2021 •
edited
Loading

in mappings in legacy templates.

It is also possible to have custom types in composable index templates. Less likely since they are newer, but still possible. I am not familiar enough with the code to know if the check in the PR covers both types of templates.

EDIT: spoke with Keith and think ^^ is incorrect since custom types require a HTTP parameter (include_type_name) to be used and don't think the composable API exposes that option.

@jakelandis
Copy link
Contributor

@jtibshirani - Would you mind also reviewing this PR ? I seem to recall some creative use _doc in the mappings based on which way you accessed it (i.e. from REST vs. internally) and just want to ensure we are looking at the right place for the mapping types given the API access.

@jakelandis jakelandis mentioned this pull request Nov 11, 2021
Closed
@masseyke
Copy link
Member Author

I'd definitely appreciate an additional review because I'm not incredibly familiar with this. For what it's worth in addition to the unit tests I added, I tested this by posting 3 templates to "_template" on a 7.16.0 server -- (1) no type, not returned by this check, (2) a type called "someType", not returned by this check, and (3) a type called "_doc", not returned by this check.

jtibshirani
jtibshirani reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

This makes sense to me, based on my memory of how these templates are stored.

.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java Outdated
state.getMetadata().getTemplates().forEach((templateCursor) -> {
String templateName = templateCursor.key;
ImmutableOpenMap<String, CompressedXContent> mappings = templateCursor.value.mappings();
if (mappings != null && mappings.size() == 1) {
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 we should check the type name even if there is more than one mapping type. For example, a user could have both a _default_ mapping and a custom one like some_type.

This makes me think -- maybe this check could be combined with the one above checkTemplatesWithMultipleTypes? That way we could report all errors at once (both that they are using a _default_ mapping, and that they have an custom type name). The error message about "multiple types" could be improved to talk about a _default_ mapping to be more specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was only checking for single mapping types with the thought that checkTemplatesWithMultipleTypes would catch the others. I'll see if I can find a clean way to combine them.

Copy link
Contributor

Choose a reason for hiding this comment

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

My earlier comment was based on a misunderstanding -- I thought you couldn't have templates with multiple custom types in 7.x, but you actually can if they are carried over from 6.x! See #72540 for details. I thought this was only possible when using the _default_ mapping, but that is not true. So no need to add special handling for _default_.

.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java Outdated
ImmutableOpenMap<String, CompressedXContent> mappings = templateCursor.value.mappings();
if (mappings != null && mappings.size() == 1) {
String typeName = mappings.iterator().next().key;
if ("_doc".equals(typeName) == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny comment, this could be MapperService.SINGLE_MAPPING_NAME.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I'd meant to go back and find that constant!

@masseyke masseyke requested a review from jtibshirani November 11, 2021 22:57
@masseyke
Copy link
Member Author

This is related to #72540

jtibshirani
jtibshirani approved these changes Nov 16, 2021
View reviewed changes
.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java Outdated
"Multiple mapping types and custom mapping types in index templates and indices are deprecated",
"https://ela.st/es-deprecation-7-multiple-types",
"Update or remove the following index templates before upgrading to 8.0: "
+ templatesWithMultipleTypes
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be allBadTemplates instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I'll fix that.

@masseyke masseyke merged commit dfe8df0 into elastic:7.16 Nov 16, 2021
@masseyke masseyke deleted the feature/deprecation-info-cutom-type-in-template branch November 16, 2021 23:08
@masseyke masseyke mentioned this pull request Nov 18, 2021
Merged
masseyke added a commit that referenced this pull request Nov 18, 2021
@masseyke
Changing deprecation info api message for templates with mappings wit…
4aef8d1 
…h a single custom type to warning level (#80858)

Custom types in templates do not cause a server to fail to come up. So this commit takes the level of the
deprecation info API message down from critical to warning.
Relates #80676
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtibshirani jtibshirani jtibshirani approved these changes

@jbaiera jbaiera Awaiting requested review from jbaiera

Assignees
No one assigned
Labels
>non-issue Team:Data Management Meta label for data/management team v7.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@masseyke @elasticmachine @jakelandis @jtibshirani @danhermann @elasticsearchmachine