Deprecate `_field_names` disabling #42854

cbuescher · 2019-06-04T14:35:35Z

Currently we allow _field_names fields to be disabled explicitely, but since
the overhead is negligible now we decided to keep it turned on by default and
deprecate and ignore the enable option on the field type.

Closes #27239

elasticmachine · 2019-06-04T14:35:37Z

Pinging @elastic/es-search

Currently we allow `_field_names` fields to be disabled explicitely, but since the overhead is negligible now we decided to keep it turned on by default and deprecate and ignore the `enable` option on the field type. Closes elastic#27239

jtibshirani

Thanks @cbuescher, it will be very nice to remove this piece of configuration. I had a couple high-level questions:

Am I understanding correctly that we are planning to deprecate the option and also start ignoring it in 7.x? I was wondering if we should follow the standard deprecation + removal process here, so that the configuration option will emit a deprecation warning in 7.x, but still work. I see that this PR takes the approach described in the original issue (Disallow disabling _field_names #27239), but to me it seems most consistent + least surprising to users to follow the normal deprecation process.
As it currently stands, I think we will log a warning on every mapping update (although it gets de-duplicated through deprecatedAndMaybeLog). This is because types are parsed every time we update mappings. If a user has specified the enabled setting, it doesn't seem like they have a way of addressing the warning unless they create a fresh mapping + reindex. I wanted to check that we're okay with this behavior.

modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java

jtibshirani · 2019-06-04T20:46:08Z

modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java

@@ -146,7 +146,7 @@ public void init() throws Exception {
        mapperService = indexService.mapperService();

        String mapper = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("doc")
-            .startObject("_field_names").field("enabled", false).endObject() // makes testing easier
+            .startObject("_field_names").endObject() // makes testing easier


Maybe we could avoid mentioning _field_names here altogether? The same thought applies to other places in this file.

+1, we should disallow _field_names entirely for newly created indices.

I'm not really following you here, #27239 talks about disabeling the enableoption. What would disallowing the whole field mean? Do we still create the field (since its default is enabled:true?) What's the whole breaking/deprecation story around this then?

Sorry I wasn't clear I meant that we should disallow setting _field_names entirely since we're on master but it seems that the scope of this pr is only the deprecation. However I agree with Julie that we should try to avoid mentioning _field_names in this test since this shouldn't affect the behavior of the percolator. If it turns too complicated we can leave this for a follow up but we'll need to do it anyway since the goal is to disallow disallowing the _field_names entirely and this test relies on this forbidden behavior.

server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java

jimczi

Thanks @cbuescher . I left some additional comments.

jimczi · 2019-06-05T07:14:13Z

modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java

@@ -146,7 +146,7 @@ public void init() throws Exception {
        mapperService = indexService.mapperService();

        String mapper = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("doc")
-            .startObject("_field_names").field("enabled", false).endObject() // makes testing easier
+            .startObject("_field_names").endObject() // makes testing easier


+1, we should disallow _field_names entirely for newly created indices.

jimczi · 2019-06-05T07:15:43Z

server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java

-                    builder.enabled(XContentMapValues.nodeBooleanValue(fieldNode, name + ".enabled"));
+                    deprecationLogger.deprecatedAndMaybeLog("field_names_enabled_parameter", ENABLED_DEPRECATION_MESSAGE);
+                    // just read the value and dump it on the floor
+                    XContentMapValues.nodeBooleanValue(fieldNode, name + ".enabled");
                    iterator.remove();


I think we should still support this field (and log the deprecation) for indices created in a version where the option is available (7x) and throw an error if the version is on or after 8.0 ?

cbuescher · 2019-06-05T09:47:00Z

Thanks for the review, I pushed a commit addressing the nits and typos.

understanding correctly that we are planning to deprecate the option and also start ignoring it in 7.x

I was wondering the same but since this PR is against 8.0 I was hoping we can discuss this here. Seeing that the scope of the issue now seems to shift slightly I think we should map out the real goals of #27239 before I proceed.

jimczi · 2019-06-05T11:42:45Z

I was wondering the same but since this PR is against 8.0 I was hoping we can discuss this here. Seeing that the scope of the issue now seems to shift slightly I think we should map out the real goals of #27239 before I proceed.

I think that most of the user of this feature (disallowing _field_names to index faster) are not aware of #26930 so they continue to disable it even though their mappings don't need to index anything in the _field_names field. However I agree with Julie that it would be least surprising + most consistent to only deprecate and not ignore especially for old indices.

As it currently stands, I think we will log a warning on every mapping update (although it gets de-duplicated through deprecatedAndMaybeLog). This is because types are parsed every time we update mappings. If a user has specified the enabled setting, it doesn't seem like they have a way of addressing the warning unless they create a fresh mapping + reindex. I wanted to check that we're okay with this behavior.

I think it's ok but if we choose to not ignore the option for old indices we should add the deprecation only if enable is set to false. This way users can remove the warning when they update their mapping to reenable the _field_names field ?

cbuescher · 2019-08-05T19:29:22Z

@jtibshirani @jimczi picking this up again, I reduced the scope of this PR to only add deprecation logging with the intent to backport this to 7.4 so we already have deprecation in 7 and then follow up in a separate PR to either completely disallow it (throwing an error) in 8 (only on newly created indices, the ones created in 7 should continue working) or just changing to a no-op in 8 and complete removal in a later version. Since this might also require some additons to the upgrade assistant I'd like to get deprecation in now and work on ignoring or removing the option entirely in a follow up PR. Wdyt?

jtibshirani · 2019-08-05T21:06:31Z

It sounds good to me to focus only on deprecation for this PR. To make sure we're on the same page before jumping into the code, what do you both think of this high-level plan?

On 7.4 we emit a deprecation warning if the mapping contains an explicit setting for 'enabled' (either true or false). We will emit this deprecation warning on 7.4 no matter when the index was created (6.x or 7.x).
On 8.0 we disallow setting enabled on new indices. The setting continues to work for indices created in 7.x, but we still emit a deprecation warning. (Or just ignore the setting on 7.x, we can discuss this in the follow-up PR).

To me this seems like the most straightforward + least surprising plan. But it is a little annoying to see a deprecation warning every time the mapping is parsed, without an easy way to fix it without creating a fresh mapping + reindexing. @jimczi suggested that we could only log a warning if enabled: false, but I was concerned that this would encourage users to set enabled: true without reindexing. Their indices would be in a confusing in-between state where enabled: true but some documents are missing _field_names information, so exists queries will return unexpected results.

cbuescher · 2019-08-09T15:24:34Z

@jtibshirani I'm in favour of the plan you proposed. I changed the PR slightly so that in its current form we emit a deprecation warning if the mapping contains an explicit setting for 'enabled' regardless of the value and the index version. I'm not too concerned with too many deprecation warnings. As far as I understand, warnings should be emitted on index creation, index opening or mapping updates (which might happen when new fields are added either via explicit mapping updates or indexing new documents with new fields). All those cases should be limited to a few times, the warnings in the request header that are always there can be ignored by the client and the warnings in the log should largely be de-duplicated.
I added information about the index that contains the "enabled" setting to make the deprecation warning more actionable.
It would be great to get the deprecation in with 7.4 but lets hear @jimczi opinion first.

cbuescher · 2019-09-09T12:42:55Z

@jtibshirani @jimczi just fixed some conflicts on this PR, looks like FF for 7.4 already passed so I think the deprecation logging can be added for 7.5 next. Could you take another look and let me know if this reflects Julies previous suggestion?
I would like to merge and backport this PR and open a follow up issue for the second point (throwing error on 8.0 on new indices), if you agree.

jtibshirani

This looks good to me as the initial deprecation PR.

server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldMapperTests.java

server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java

cbuescher · 2019-09-11T12:55:24Z

Thanks @jtibshirani, also checked with @jimczi on another channel who approves the plan as well. Will merge then and follow up with another PR on master to disallow the setting on new indices.

Currently we allow `_field_names` fields to be disabled explicitely, but since the overhead is negligible now we decided to keep it turned on by default and deprecate the `enable` option on the field type. This change adds a deprecation warning whenever this setting is used, going forward we want to ignore and finally remove it. Closes #27239

After deprecating the setting starting with 7.5 in elastic#42854, this change disallows setting enabled for the `_field_names` field on new indices. The setting continues to work for indices created in 7.x where we still emit a deprecation warning.

After deprecating the `enabled` setting for `_field_names`starting with 7.5, this change disallows the setting on new indices in 8.0. The setting continues to work for indices created in 7.x where we continue emitting a deprecation warning. Relates to #42854

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

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 warning for templates containing this setting, which has been removed with 8.0. Relates to #42854, #46681

cbuescher added >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.2.0 v7.3.0 labels Jun 4, 2019

Deprecate and ignore _field_names disabling

9635484

Currently we allow `_field_names` fields to be disabled explicitely, but since the overhead is negligible now we decided to keep it turned on by default and deprecate and ignore the `enable` option on the field type. Closes elastic#27239

cbuescher force-pushed the fix-27239 branch from 63d3585 to 9635484 Compare June 4, 2019 15:44

jtibshirani reviewed Jun 4, 2019

View reviewed changes

jimczi requested changes Jun 5, 2019

View reviewed changes

iter

26e8b64

cbuescher force-pushed the fix-27239 branch from 03e3b13 to 26e8b64 Compare June 5, 2019 10:04

jakelandis removed the v7.2.0 label Jun 17, 2019

jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019

Christoph Büscher added 2 commits August 5, 2019 16:52

Merge branch 'master' into fix-27239

e4620c6

Only deprecate and log

d46bf47

cbuescher changed the title ~~Deprecate and ignore _field_names disabling~~ Deprecate _field_names disabling Aug 5, 2019

Christoph Büscher added 2 commits August 9, 2019 15:49

Merge branch 'master' into fix-27239

c2e5503

Log deprecation on any index version, inlcude name in log

4308e8e

Christoph Büscher added 2 commits August 9, 2019 19:51

iter

b653c5d

Merge branch 'master' into fix-27239

96ef8f8

colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019

Merge branch 'master' into fix-27239

c4a0cb0

jtibshirani approved these changes Sep 9, 2019

View reviewed changes

server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldMapperTests.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java Outdated Show resolved Hide resolved

Changes to deprecation message and revert removing test

c552702

cbuescher merged commit d0a7bbc into elastic:master Sep 11, 2019

cbuescher mentioned this pull request Sep 12, 2019

Disallow _field_names enabled setting #46681

Merged

cbuescher added the >deprecation label Sep 20, 2019

cbuescher mentioned this pull request Sep 23, 2019

Add migration tool checks for _field_names disabling #46972

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `_field_names` disabling #42854

Deprecate `_field_names` disabling #42854

cbuescher commented Jun 4, 2019

elasticmachine commented Jun 4, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Jun 4, 2019

jimczi Jun 5, 2019

cbuescher Jun 5, 2019

jimczi Jun 5, 2019

jimczi left a comment

jimczi Jun 5, 2019

jimczi Jun 5, 2019

cbuescher commented Jun 5, 2019

jimczi commented Jun 5, 2019

cbuescher commented Aug 5, 2019

jtibshirani commented Aug 5, 2019

cbuescher commented Aug 9, 2019

cbuescher commented Sep 9, 2019

jtibshirani left a comment

cbuescher commented Sep 11, 2019

Deprecate _field_names disabling #42854

Deprecate _field_names disabling #42854

Conversation

cbuescher commented Jun 4, 2019

elasticmachine commented Jun 4, 2019

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

jtibshirani Jun 4, 2019

Choose a reason for hiding this comment

jimczi Jun 5, 2019

Choose a reason for hiding this comment

cbuescher Jun 5, 2019

Choose a reason for hiding this comment

jimczi Jun 5, 2019

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

jimczi Jun 5, 2019

Choose a reason for hiding this comment

jimczi Jun 5, 2019

Choose a reason for hiding this comment

cbuescher commented Jun 5, 2019

jimczi commented Jun 5, 2019

cbuescher commented Aug 5, 2019

jtibshirani commented Aug 5, 2019

cbuescher commented Aug 9, 2019

cbuescher commented Sep 9, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

cbuescher commented Sep 11, 2019

Deprecate `_field_names` disabling #42854

Deprecate `_field_names` disabling #42854

jtibshirani left a comment •

edited

Loading