Cache ILM policy name on IndexMetadata #83603

joegallo · 2022-02-07T16:50:12Z

One little detail is that the LIFECYCLE_NAME value is defined on IndexMetadata but it's still almost entirely referenced by way of LifecycleSettings. We need to have the value available in IndexMetadata in order to pull the value from the settings and set it into lifecyclePolicyName, but besides that everything about this still seems more LifecycleSettings-related to me.

for symmetry with all the rest of the Setting.whateverSetting calls here.

rather than repeatedly looking it up in the settings all the time

in favor of just using LIFECYCLE_NAME

elasticmachine · 2022-02-07T16:50:15Z

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

elasticsearchmachine · 2022-02-07T16:50:37Z

Hi @joegallo, I've created a changelog YAML for you.

original-brownbear

Thanks Joe! just one quick question.

original-brownbear · 2022-02-07T17:34:40Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleSettings.java

@@ -35,7 +34,7 @@
    // already mounted as a searchable snapshot. Those ILM actions will check if the index has this setting name configured.
    public static final String SNAPSHOT_INDEX_NAME = "index.store.snapshot.index_name";

-    public static final Setting<TimeValue> LIFECYCLE_POLL_INTERVAL_SETTING = timeSetting(
+    public static final Setting<TimeValue> LIFECYCLE_POLL_INTERVAL_SETTING = Setting.timeSetting(


This seems unrelated?

It's just a cleanup commit. "While I was here", so to speak.

original-brownbear · 2022-02-07T17:36:28Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

@@ -1642,6 +1656,7 @@ public IndexMetadata build() {
                DiskThresholdDecider.SETTING_IGNORE_DISK_WATERMARKS.get(settings),
                tierPreference,
                ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.get(settings),
+                settings.get(IndexMetadata.LIFECYCLE_NAME),


This seems mildly hacky, should we just move the full Setting instance in here so we can just use LIFECYCLE_NAME_SETTING.get(settings) the standard way?

This has the null semantics I want, but the standard way doesn't -- SOME_SETTING.get(settings) returns the default value for the setting if the setting is absent, and in this case that's an empty string (because you can't define the default value for a setting as being null).

Of course, that's not impossible to work around, we could do a hasText check or something like that.

Ah I see. This is mildly scary because we seem to have been mixing both patterns for this setting before and now we're only using one that has null in it.
But I guess we use if (Strings.isNullOrEmpty(policyName) == false) { or the return as the right hand side of an equals so it should be good.

Prior to this PR, this condition was not correct, and would never have been triggered -- it used: LifecycleSettings.LIFECYCLE_NAME_SETTING.get(indexMetadata.getSettings()) to read the policy name, and the .get there would return the default value for the setting (empty string) if the setting was not present, *not* null. So this condition would always see a non-null policyName and never actually did anything. We have some tests that exercise this scenario, though, and they expect the error message that we throw from IndexLifecycleTransition.validateTransition -- by throwing with that same error message here those tests pass and the shortcut condition here actually works as expected.

original-brownbear

LGTM (maybe add the annotation though, makes everyone's life easier). Thanks Joe!

original-brownbear · 2022-02-07T19:54:28Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

@@ -1642,6 +1656,7 @@ public IndexMetadata build() {
                DiskThresholdDecider.SETTING_IGNORE_DISK_WATERMARKS.get(settings),
                tierPreference,
                ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.get(settings),
+                settings.get(IndexMetadata.LIFECYCLE_NAME),


Ah I see. This is mildly scary because we seem to have been mixing both patterns for this setting before and now we're only using one that has null in it.
But I guess we use if (Strings.isNullOrEmpty(policyName) == false) { or the return as the right hand side of an equals so it should be good.

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

It would have NPE-ed on null, and AFAICT it would never have been called with an empty string (and we already check for spaces).

This class already uses hasText for this elsewhere, might as well use it consistently (it's shorter and clearer this way).

* upstream/master: (166 commits) Bind host all instead of just _site_ when needed (elastic#83145) [DOCS] Fix min/max agg snippets for histograms (elastic#83695) [DOCS] Add deprecation notice for system indices (elastic#83688) Cache ILM policy name on IndexMetadata (elastic#83603) [DOCS] Fix 8.0 breaking changes sort order (elastic#83685) [ML] fix random sampling background query consistency (elastic#83676) Move internal APIs into their own namespace '_internal' Runtime fields core-with-mapped tests support tsdb (elastic#83577) Optimize calculating the presence of a quorum (elastic#83638) Use switch expressions in EnableAllocationDecider and NodeShutdownAllocationDecider (elastic#83641) Note libffi error message in tmpdir docs (elastic#83662) Fix TransportDesiredNodesActionsIT batch tests (elastic#83659) [DOCS] Remove unused upgrade doc files (elastic#83617) [ML] Wait for model process to stop in stop deployment (elastic#83644) [ML] Fix submit after shutdown in process worker service (elastic#83645) Remove req/resp classes associated with HLRC (elastic#83599) Introduce index.version.compatibility setting (elastic#83264) Rename InternalTestCluster#getMasterNodeInstance (elastic#83407) Mute TimeSeriesIndexSearcherTests testCollectInOrderAcrossSegments (elastic#83648) Add rollover add max_primary_shard_docs condition (elastic#80981) ... # Conflicts: # x-pack/plugin/rollup/build.gradle # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java

joegallo added 5 commits February 7, 2022 11:33

Use policyName more consistently as a variable name

306e01c

Use Setting.timeSetting

6611dc2

for symmetry with all the rest of the Setting.whateverSetting calls here.

Hoist the ILM lifecycle policy name to IndexMetadata

0241c70

rather than repeatedly looking it up in the settings all the time

Mark as nullable, add a test

ac72446

Avoid LIFECYCLE_NAME_SETTING.getKey()

85119a6

in favor of just using LIFECYCLE_NAME

joegallo added >enhancement :Data Management/ILM+SLM Index and Snapshot lifecycle management v8.2.0 labels Feb 7, 2022

elasticmachine added the Team:Data Management Meta label for data/management team label Feb 7, 2022

Update docs/changelog/83603.yaml

5b263dc

joegallo requested a review from original-brownbear February 7, 2022 16:53

original-brownbear reviewed Feb 7, 2022

View reviewed changes

joegallo added 2 commits February 7, 2022 12:42

Merge branch 'master' into hoist-lifecycle-name-setting

d3c96ea

original-brownbear approved these changes Feb 7, 2022

View reviewed changes

joegallo added 6 commits February 7, 2022 17:05

Add nullable, and some explanations

508447c

Merge branch 'master' into hoist-lifecycle-name-setting

efb6eb9

More explicit policy name validation

280b43b

It would have NPE-ed on null, and AFAICT it would never have been called with an empty string (and we already check for spaces).

Invert this check

db05f03

This class already uses hasText for this elsewhere, might as well use it consistently (it's shorter and clearer this way).

Merge branch 'master' into hoist-lifecycle-name-setting

343699d

Merge branch 'master' into hoist-lifecycle-name-setting

d68db24

joegallo merged commit c182b00 into elastic:master Feb 8, 2022

joegallo deleted the hoist-lifecycle-name-setting branch February 8, 2022 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cache ILM policy name on IndexMetadata #83603

Cache ILM policy name on IndexMetadata #83603

Uh oh!

joegallo commented Feb 7, 2022 •

edited

Loading

Uh oh!

elasticmachine commented Feb 7, 2022

Uh oh!

elasticsearchmachine commented Feb 7, 2022

Uh oh!

original-brownbear left a comment

Uh oh!

original-brownbear Feb 7, 2022

Uh oh!

joegallo Feb 7, 2022

Uh oh!

original-brownbear Feb 7, 2022

Uh oh!

joegallo Feb 7, 2022

Uh oh!

original-brownbear Feb 7, 2022

Uh oh!

original-brownbear left a comment

Uh oh!

original-brownbear Feb 7, 2022

Uh oh!

Uh oh!

Uh oh!

Cache ILM policy name on IndexMetadata #83603

Cache ILM policy name on IndexMetadata #83603

Uh oh!

Conversation

joegallo commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 7, 2022

Uh oh!

elasticsearchmachine commented Feb 7, 2022

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

joegallo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

original-brownbear Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

joegallo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

original-brownbear Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joegallo commented Feb 7, 2022 •

edited

Loading