Refactor CleanupShrinkIndexStep to CleanupGeneratedIndexStep
#133356
Conversation
By making the step more generic, we can reuse it in other actions (such as the upcoming improved force merge action). We ensure that indices that are in this cleanup step while the cluster upgrades to this version, are still able to progress seamlessly by registering the old step name as a `NoopStep` and forwarding them to the new cleanup step name.
nielsbauman
added
>refactoring
auto-backport
elasticsearchmachine
added
needs:triage
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CleanupShrinkIndexStep to CleanupGeneratedIndexStep to make it more generic and reusable for other actions beyond just shrink operations.
Key Changes
- Renamed the step class and updated its functionality to use a configurable target index name supplier
- Added backward compatibility by introducing a
NoopStepthat forwards indices from the old step name to the new one - Updated all test cases to reflect the new class name and structure
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ShrinkAction.java | Integrates the new generic cleanup step with a NoopStep for backward compatibility |
| CleanupGeneratedIndexStep.java | Refactored from CleanupShrinkIndexStep with configurable index name supplier |
| CleanupGeneratedIndexStepTests.java | Updated test class to match the new generic step implementation |
| ShrinkActionTests.java | Updated test assertions to account for the additional NoopStep in the workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CleanupGeneratedIndexStep.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
nielsbauman
added
the
:Data Management/ILM+SLM
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
elasticsearchmachine
removed
the
needs:triage
|
Build is failing due to unrelated compilation issues, will be addressed by #133354. |
nielsbauman
removed
auto-backport
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CleanupGeneratedIndexStep.java
Outdated
Show resolved
Hide resolved
| StepKey oldCleanupShrinkIndexKey = new StepKey(phase, NAME, CleanupGeneratedIndexStep.OLD_NAME); | ||
| StepKey cleanupShrinkIndexKey = new StepKey(phase, NAME, CleanupGeneratedIndexStep.NAME); |
There was a problem hiding this comment.
I just realized that instead of having this old noop step, we could just move the OLD_NAME field to this class and keep using that name for the cleanup step. I don't think there's much value in making this action using the new name too, as we'll never be able to remove the noop step.
There was a problem hiding this comment.
I removed the NoopStep I added and just moved the OLD_NAME field to the CLEANUP_SHRINK_INDEX_STEP field in ShrinkAction. That avoids us having to keep the noop step forever.
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CleanupGeneratedIndexStep.java
Outdated
Show resolved
Hide resolved
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CleanupGeneratedIndexStep.java
Outdated
Show resolved
Hide resolved
3 participants
By making the step more generic, we can reuse it in other actions (such
as the upcoming improved force merge action).