-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Refactor ILMs UpdateSettingsStep to be more generic
#133329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
By making the step more generic, we can reuse it in other actions (such as the upcoming improvements to the force-merge action). The change is relatively straightforward as the name of the step is already generic enough.
|
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the UpdateSettingsStep class to use function suppliers instead of static settings, making it more generic and reusable for other ILM actions like the upcoming force-merge improvements.
- Replaces static
Settingsfield withFunction<IndexMetadata, Settings>supplier for dynamic settings generation - Adds support for customizable target index name via
Function<IndexMetadata, String>supplier - Updates all test files to use the new
getSettingsSupplier().apply(null)pattern instead ofgetSettings()
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| UpdateSettingsStep.java | Main refactoring - replaces static settings with function suppliers and removes equals/hashCode methods |
| UpdateSettingsStepTests.java | Updates test methods to use new supplier-based API and removes settings mutation test case |
| SetPriorityActionTests.java | Updates test assertions to call settings supplier instead of direct settings access |
| MigrateActionTests.java | Updates test assertions to use settings supplier pattern |
| ForceMergeActionTests.java | Updates test assertion to use settings supplier |
| AllocateActionTests.java | Updates test assertions and extracts settings to variables for cleaner code |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java
Show resolved
Hide resolved
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStepTests.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not much value in overriding hashCode and equals when the only two fields are functions (which might be lambdas) and thus do not implement those methods.
| } | ||
|
|
||
| assertThat(firstStep.getSettings(), equalTo(expectedSettings.build())); | ||
| assertThat(firstStep.getSettingsSupplier().apply(null), equalTo(expectedSettings.build())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered adding a getter like so in UpdateSettingsStep:
// visibility for testing
Settings getSettings() {
return settingsApplier(null);
}to avoid having to do this in several tests, but I didn't feel like that was a good place for such test-only code, and I couldn't think of a sensible other place, so decided to update the few tests that required updating.
dakrone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left one super minor comment
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateSettingsStep.java
Show resolved
Hide resolved
| private final Settings settings; | ||
| private static final BiFunction<String, LifecycleExecutionState, String> DEFAULT_TARGET_INDEX_NAME_SUPPLIER = (index, state) -> index; | ||
|
|
||
| private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the signature of the field to match the signature of similar fields in other steps, such as:
Line 38 in cb2e4db
| private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier; |
While I would prefer using
Function<IndexMetadata, String> which still gives access to the name and state, and is more compact, having only one ShrinkIndexNameSupplier#getShrinkIndexName is more worth it IMO. We can always refactor all usages if one go if we change our mind.
By making the step more generic, we can reuse it in other actions (such as the upcoming improvements to the force-merge action). The change is relatively straightforward as the name of the step is already generic enough.
By making the step more generic, we can reuse it in other actions (such as the upcoming improvements to the force-merge action).
The change is relatively straightforward as the name of the step is already generic enough.