Add migration tool checks for _field_names disabling #46972
Conversation
cbuescher
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
|
@jtibshirani since you already reviewed #46681 that adds throwing an error on 8.0 for this setting, maybe you can also take a look at this PR that adds checks for existing mappings and templates to the migration assistant? |
This change adds a check to the migration tool that warns about the deprecated "enabled" setting for the "_field_names" field on 7.x indices and issues a critical error for templates containing this setting, since it has been removed with 8.0. Relates to elastic#42854, elastic#46681
cbuescher
force-pushed
the
fieldnames_disabling_deprecationcheck
branch
from
e894414 to
b3e5098
Compare
.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java
Outdated
Show resolved
Hide resolved
...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java
Outdated
Show resolved
Hide resolved
...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java
Show resolved
Hide resolved
| /** | ||
| * check for "_field_names" entries in the map that contain another property "enabled" in the sub-map | ||
| */ | ||
| static boolean mapContainsFieldNamesDisabled(Map<?, ?> map) { |
There was a problem hiding this comment.
Do we need to use a recursive function here, or could we just check for it directly since we know _field_names will be at the top level of the mapping? Also, there is a convenient method IndexDeprecationChecks#findInPropertiesRecursively for iterating over a mapping.
There was a problem hiding this comment.
I opted for the recursive function just to make sure we catch all type-vs-no-type cases (see also the test cases). If we always have (or don't have) a type-level in the internally stored mappings in the templates, then we can probably do without but I wanted to check with you on this, it wasn't completely clear to me from reading the code. The mapping maps are quite forgiving for what you store in them, so it depends on the caller and I didn't completely follow the internal logic.
there is a convenient method
IndexDeprecationChecks#findInPropertiesRecursively
I saw and tried to use this, but ran into problems because it assumes things are stored under a properties key which I think the "_field_nams" : { "enabled" : ...} is not (correct me if I'm wrong).
Let me if you know for sure how the internal map structure is supposed to look like (at which level to expect the "_field_names" key) and I can update the tests accordingly and try making this non-recursive.
There was a problem hiding this comment.
I saw and tried to use this, but ran into problems because it assumes things are stored under a properties key
Ah, I missed this -- then findInPropertiesRecursively is indeed not a good fit.
Let me if you know for sure how the internal map structure is supposed to look like
I left some comments on your individual questions, let me know if anything is still unclear!
|
@jtibshirani I verified my assumptions about what the mappings/templates maps really contain in a local cluster with and without explicit type definitions a bit further and added 67e44ae as another commit. I will add a few remarks / questions where I'm unsure my understanding is correct, maybe you can verify those. |
.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java
Outdated
Show resolved
Hide resolved
...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java
Show resolved
Hide resolved
|
@elasticmachine run elasticsearch-ci/default-distro |
|
Thanks for the comments, I hope f00a419 solves the edge case you mentioned. |
.../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine run elasticsearch-ci/default-distro |
This change adds a check to the migration tool that warns about the deprecated
"enabled" setting for the "_field_names" field on 7.x indices and issues a
critical error for templates containing this setting, since it has been removed
with 8.0.
Relates to #42854, #46681