Cache ILM policy name on IndexMetadata#83603
Conversation
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
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @joegallo, I've created a changelog YAML for you. |
original-brownbear
left a comment
There was a problem hiding this comment.
Thanks Joe! just one quick question.
| 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( |
There was a problem hiding this comment.
This seems unrelated?
There was a problem hiding this comment.
It's just a cleanup commit. "While I was here", so to speak.
| DiskThresholdDecider.SETTING_IGNORE_DISK_WATERMARKS.get(settings), | ||
| tierPreference, | ||
| ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.get(settings), | ||
| settings.get(IndexMetadata.LIFECYCLE_NAME), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
LGTM (maybe add the annotation though, makes everyone's life easier). Thanks Joe!
| DiskThresholdDecider.SETTING_IGNORE_DISK_WATERMARKS.get(settings), | ||
| tierPreference, | ||
| ShardsLimitAllocationDecider.INDEX_TOTAL_SHARDS_PER_NODE_SETTING.get(settings), | ||
| settings.get(IndexMetadata.LIFECYCLE_NAME), |
There was a problem hiding this comment.
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
Show resolved
Hide resolved
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
Closes #83582
One little detail is that the
LIFECYCLE_NAMEvalue 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 intolifecyclePolicyName, but besides that everything about this still seems moreLifecycleSettings-related to me.