-
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
Remove index mapper dynamic settings #25734
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
@PnPie Ideally we would only remove this setting for 6.0 indices, and keep it working for 5.x indices. |
@jpountz you self-requested a review, is there more reviewing for this or does it need to be changed for working with 6.0 indices and removed for 7.0 indices as you commented? |
@dakrone It needs to be changed to take the creation version into account indeed. I self requested a review to make sure I would get a notification if this PR was updated. |
@jpountz @dakrone srry for the late response, I didn't well understand that @jpountz mentioned in fact, could you please explain a bit more about the modification and what does it mean "to take the creation version into account" ? I'm on vacation these days and maybe I cannot change it very quickly. |
@PnPie We would like to keep this feature working for indices that have been created with Elasticsearch 5.x. So the idea would be to undo your changes completely, replace this.dynamic = this.indexSettings.getValue(INDEX_MAPPER_DYNAMIC_SETTING); with something like if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_6_0_0)) {
if (INDEX_MAPPER_DYNAMIC_SETTING.exists(indexSettings.getSettings())) {
throw new IllegalArgumentException("Setting [" + INDEX_MAPPER_DYNAMIC_SETTING.getKey() + "] is removed as of 6.0.0 since it became useless with the removal of types");
}
this.dynamic = INDEX_MAPPER_DYNAMIC_DEFAULT;
} else {
this.dynamic = this.indexSettings.getValue(INDEX_MAPPER_DYNAMIC_SETTING);
} Then tests need to be fixed in order to set the creation version explicitly so that they can keep testing this functionality. And ideally we'd need a new test that makes sure that we fail index creation if this setting is set. |
Remove "index.mapper.dynamic" setting for 6.0 (and after) indices, but still keep working for 5.x (and before) indices. Remove two index dynamic disable test cases as the disability of index.mapper.dynamic is already removed for current version. Add a new test class for version test.
b78bde2
to
f087e1e
Compare
@jpountz I did the modification as mentioned and I removed 2 test cases for the disability of `index.mapper.dynamic" at cluster level as it is already removed in current version (and it seems setting index version explicitly for these 2 cases is a little bit complicated ?). I also added a version test class to test the setting in current and 5.0 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.
I left a suggestion.
this.dynamic = INDEX_MAPPER_DYNAMIC_DEFAULT; | ||
} else { | ||
this.dynamic = this.indexSettings.getValue(INDEX_MAPPER_DYNAMIC_SETTING); | ||
} |
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.
master is the future 7.0, so I would do the following:
if (INDEX_MAPPER_DYNAMIC_SETTING.exists(indexSettings.getSettings())) {
if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_7_0_0)) {
// 7.x index
throw new IllegalArgumentException("Setting " + INDEX_MAPPER_DYNAMIC_SETTING.getKey() + " was removed after version 6.0.0");
} else {
// 6.x index
DEPRECATION_LOGGER.deprecated("Setting " + INDEX_MAPPER_DYNAMIC_SETTING.getKey() + " is deprecated since indices may not have more than one type anymore.");
}
}
Then when backporting I'll just remove the 7.x branch and make sure that we only emit a deprecation warning on 6.x indices (you don't need to worry about it).
Throw an exception on 7.0 version and emit a deprecation logging message for 6.x indices.
@elasticmachine please test it |
1 similar comment
@elasticmachine please test it |
The failing test passes when I rebase. I'll merge. Thanks @PnPie ! |
Remove "index.mapper.dynamic" setting for 6.0 (and after) indices, but still keep working for 5.x (and before) indices. Remove two index dynamic disable test cases as the disability of index.mapper.dynamic is already removed for current version. Add a new test class for version test.
Remove "index.mapper.dynamic" setting for 6.0 (and after) indices, but still keep working for 5.x (and before) indices. Remove two index dynamic disable test cases as the disability of index.mapper.dynamic is already removed for current version. Add a new test class for version test.
…rflow * origin/master: (59 commits) Fix Lucene version of 5.6.1. Remove azure deprecated settings (elastic#26099) Handle the 5.6.0 release Allow plugins to validate cluster-state on join (elastic#26595) Remove index mapper dynamic settings (elastic#25734) update AWS SDK for ECS Task IAM support in discovery-ec2 (elastic#26479) Azure repository: Accelerate the listing of files (used in delete snapshot) (elastic#25710) Build: Remove norelease from forbidden patterns (elastic#26592) Fix reference to painless inside expression engine (elastic#26528) Build: Move javadoc linking to root build.gradle (elastic#26529) Test: Remove leftover static bwc test case (elastic#26584) Docs: Remove remaining references to file and native scripts (elastic#26580) Snapshot fallback should consider build.snapshot elastic#26496: Set the correct bwc version after backport to 6.x Fix the MapperFieldType.rangeQuery API. (elastic#26552) Deduplicate `_field_names`. (elastic#26550) [Docs] Update method setSource(byte[] source) (elastic#26561) [Docs] Fix typo in javadocs (elastic#26556) Allow multiple digits in Vagrant 2.x minor versions Support Vagrant 2.x ...
index.mapper.dynamic should not be used for 6.0+ indices, so this commit removes it from the esArchiver mappings.json for kibana Related elastic/elasticsearch#25734
index.mapper.dynamic should not be used for 6.0+ indices, so this commit removes it from the esArchiver mappings.json for kibana Related elastic/elasticsearch#25734
index.mapper.dynamic should not be used for 6.0+ indices, so this commit removes it from the esArchiver mappings.json for kibana Related elastic/elasticsearch#25734
index.mapper.dynamic should not be used for 6.0+ indices, so this commit removes it from the esArchiver mappings.json for kibana Related elastic/elasticsearch#25734
* master: (21 commits) Ensure module is bundled before installing in tests Add boolean similarity to built in similarity types (elastic#26613) [Tests] Remove skip tests in search/30_limits.yml Let search phases override max concurrent requests Add a soft limit for the number of requested doc-value fields (elastic#26574) Support for accessing Azure repositories through a proxy (elastic#23518) Add beta tag to MSI Windows Installer (elastic#26616) Fix Lucene version of 5.6.1. Remove azure deprecated settings (elastic#26099) Handle the 5.6.0 release Allow plugins to validate cluster-state on join (elastic#26595) Remove index mapper dynamic settings (elastic#25734) update AWS SDK for ECS Task IAM support in discovery-ec2 (elastic#26479) Azure repository: Accelerate the listing of files (used in delete snapshot) (elastic#25710) Build: Remove norelease from forbidden patterns (elastic#26592) Fix reference to painless inside expression engine (elastic#26528) Build: Move javadoc linking to root build.gradle (elastic#26529) Test: Remove leftover static bwc test case (elastic#26584) Docs: Remove remaining references to file and native scripts (elastic#26580) Snapshot fallback should consider build.snapshot ...
@jpountz this is a breaking change right? Would you mind adding labels? Thanks! |
* es_mapping: update turning off dynamic mappings they changed it in 6.x https://www.elastic.co/guide/en/elasticsearch/reference/current/dynamic.html elastic/elasticsearch#25734 * es_mapping: remove _all field deprecated in 6.0 anyway * es_mapping.yml: fix deprecated mapping type https://www.elastic.co/guide/en/elasticsearch/reference/6.7/removal-of-types.html#_schedule_for_removal_of_mapping_types it gives a really unhelpful error otherwise, oof. * es: fix remaining 7.xisms the enabled: false apparently only applies to "object" fields now, need index: false and the _type got removed everywhere. Seems to work now. * Fix weird offset error with word_delimiter_graph yet another es7-ism i guess * Fix warning and some app stuff for ES 7.x Co-authored-by: Arylide <[email protected]>
* es_mapping: update turning off dynamic mappings they changed it in 6.x https://www.elastic.co/guide/en/elasticsearch/reference/current/dynamic.html elastic/elasticsearch#25734 * es_mapping: remove _all field deprecated in 6.0 anyway * es_mapping.yml: fix deprecated mapping type https://www.elastic.co/guide/en/elasticsearch/reference/6.7/removal-of-types.html#_schedule_for_removal_of_mapping_types it gives a really unhelpful error otherwise, oof. * es: fix remaining 7.xisms the enabled: false apparently only applies to "object" fields now, need index: false and the _type got removed everywhere. Seems to work now. * Fix weird offset error with word_delimiter_graph yet another es7-ism i guess * Fix warning and some app stuff for ES 7.x Co-authored-by: Arylide <[email protected]>
This setting was removed via elastic#25734, because the setting no longer used since 6.0.0 However, the validation only kicked when trying to set this setting on a closed index. Applying the setting on an open index would just work. With severe consequences later on. For example when upgrading the cluster, nodes would refuse to boot, because the validation would kick in.
This setting was removed via #25734, because the setting no longer used since 6.0.0 However, the validation only kicked when trying to set this setting on a closed index. Applying the setting on an open index would just work. With severe consequences later on. For example when upgrading the cluster, nodes would refuse to boot, because the validation would kick in. Relates to #96075
Remove
index.mapper.dynamic
setting.Relates to #25719