Speed up ILM cluster task execution #85405
Merged
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
The other ilm UpdateTasks call it idxMeta in the same context (for better or worse).
This is only for updating an indexMetadata that already exists and is associated with the cluster state, it's not for sneakily injecting new indices.
Since the index already exists, and the lifecycleState is the only thing that's being changed, we can bypass most of the expense of building a new cluster state (the internal validation that Metadata.Builder runs).
in order to prevent the non-performant code path from resembling the performant code path. InitializePolicyContextStep now explicitly uses the non-performant code path, while all the other callers still use the performant path.
joegallo
added
>enhancement
:Data Management/ILM+SLM
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @joegallo, I've created a changelog YAML for you. |
dakrone
approved these changes
Mar 28, 2022
| @@ -299,6 +300,47 @@ public Metadata withIncrementedVersion() { | |||
| ); | |||
| } | |||
|
|
|||
| public Metadata withLifecycleState(final Index index, final LifecycleExecutionState lifecycleState) { | |||
There was a problem hiding this comment.
Since this is a public method, can you add javadoc for it?
There was a problem hiding this comment.
How does ea9c861 look to you?
| final ImmutableOpenMap.Builder<String, IndexMetadata> builder = ImmutableOpenMap.builder(indices); | ||
| builder.put(index.getName(), indexMetadataBuilder.build()); | ||
|
|
||
| return new Metadata( |
There was a problem hiding this comment.
I think it might be worth adding a comment about why this doesn't use Metadata.builder(this).etc(...) but directly constructs the Metadata, so that it doesn't get accidentally undone in the future. What do you think?
There was a problem hiding this comment.
I handled this in ea9c861, too.
...plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateUtils.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ExecuteStepsUpdateTask.java
Outdated
Show resolved
Hide resolved
dakrone
approved these changes
Mar 29, 2022
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.
Closes #82708
The central change here is the addition of the special purpose
Metadata#withLifecycleStatemethod that bypasses some of the expensive validation that we perform inMetadata.Builder#build-- since we're not adding or removing indices, that validation isn't relevant.In the many shards scalability profiling that @original-brownbear did against this code, it decreased ILM execution time from 93.33% of the profile to less than 1%, so it's extraordinarily effective at decreasing the time the master spends executing the ILM cluster task batches.