Add a cluster setting to disallow loading fielddata on _id field #49166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jimczi
added
>enhancement
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
jimczi
force-pushed
the
disable_id_fielddata
branch
from
e85c14e to
cf6d8ca
Compare
jpountz
approved these changes
Nov 20, 2019
server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java
Outdated
Show resolved
Hide resolved
| @@ -142,9 +143,18 @@ | |||
|
|
|||
| final MapperRegistry mapperRegistry; | |||
|
|
|||
| private final BooleanSupplier idFieldDataEnabled; | |||
|
|
|||
| public MapperService(IndexSettings indexSettings, IndexAnalyzers indexAnalyzers, NamedXContentRegistry xContentRegistry, | |||
| SimilarityService similarityService, MapperRegistry mapperRegistry, | |||
| Supplier<QueryShardContext> queryShardContextSupplier) { | |||
There was a problem hiding this comment.
can we remove this constructor and change callers to pass () -> false, e.g. in tests?
| @@ -168,6 +169,9 @@ | |||
| public static final String INDICES_SHARDS_CLOSED_TIMEOUT = "indices.shards_closed_timeout"; | |||
| public static final Setting<TimeValue> INDICES_CACHE_CLEAN_INTERVAL_SETTING = | |||
| Setting.positiveTimeSetting("indices.cache.cleanup_interval", TimeValue.timeValueMinutes(1), Property.NodeScope); | |||
| public static final Setting<Boolean> INDICES_ID_FIELD_DATA_ENABLED_SETTING = | |||
| Setting.boolSetting("indices.id_field_data.enabled", false, Property.Dynamic, Property.NodeScope); | |||
There was a problem hiding this comment.
if I understand correctly, the default will become true upon backport?
|
This is amazing! I can really use this.. I have users that accidentally try to aggregate on _id, and it destroys my memory.. 1.5gb used until I clear the cache for it. |
This change adds a dynamic cluster setting named `indices.id_field_data.enabled`. When set to `false` any attempt to load the fielddata for the `_id` field will fail with an exception. The default value in this change is set to `false` in order to prevent fielddata usage on this field for future versions but it will be set to `true` when backporting to 7x. When the setting is set to true (manually or by default in 7x) the loading will also issue a deprecation warning since we want to disallow fielddata entirely when elastic#26472 is implemented. Closes elastic#43599
…per.java Rewording Co-Authored-By: Adrien Grand <[email protected]>
jimczi
force-pushed
the
disable_id_fielddata
branch
from
30844dd to
e8aa2c4
Compare
jpountz
approved these changes
Nov 25, 2019
cbuescher
pushed a commit
to cbuescher/elasticsearch
that referenced
this pull request
Nov 27, 2019
The test was slightly modified with elastic#49166, the two test documents in `testNormalization` look like they should mirror the document id in the "id" field in order for it to work as a tie breaker. Closes elastic#49654
cbuescher
pushed a commit
that referenced
this pull request
Nov 27, 2019
jimczi
added a commit
that referenced
this pull request
Nov 28, 2019
) This change adds a dynamic cluster setting named `indices.id_field_data.enabled`. When set to `false` any attempt to load the fielddata for the `_id` field will fail with an exception. The default value in this change is set to `false` in order to prevent fielddata usage on this field for future versions but it will be set to `true` when backporting to 7x. When the setting is set to true (manually or by default in 7x) the loading will also issue a deprecation warning since we want to disallow fielddata entirely when #26472 is implemented. Closes #43599
jimczi
added a commit
that referenced
this pull request
Nov 28, 2019
This was referenced Nov 28, 2019
Closed
graphaelli
added a commit
to graphaelli/apm-integration-testing
that referenced
this pull request
Dec 16, 2019
as of elastic/elasticsearch#49166 it is disabled by default. The kibana task manager is upset about thi so reenable globally for now.
This was referenced Feb 3, 2020
|
@jimczi , shouldn't we add the new setting in this document as a reference? https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-id-field.html It's the doc where we recommend against sorting or aggregating towards
|
|
Would be awesome to backport this to 6.8 |
|
Sorry @nezda but we won’t backport this to 6.8 because our policy is that we don’t add new features to versions in maintenance mode. |
|
There are no reference docs on our site about this setting |
This was referenced Nov 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds a dynamic cluster setting named
indices.id_field_data.enabled.When set to
falseany attempt to load the fielddata for the_idfield will failwith an exception. The default value in this change is set to
falsein order to preventfielddata usage on this field for future versions but it will be set to
truewhen backportingto 7x. When the setting is set to true (manually or by default in 7x) the loading will also issue
a deprecation warning since we want to disallow fielddata entirely when #26472 is implemented.
Closes #43599