Makes StepRule more configurable #96855
Conversation
Allows having different StepRule shapes: - only check maxRetries - only check maxTimeOn - check both adding specific factory methods
HiDAl
added
>refactoring
:Data Management/ILM+SLM
|
Pinging @elastic/es-data-management (Team:Data Management) |
HiDAl
force-pushed
the
make-steps-retries-configurable
branch
from
a4f3656 to
0adfc46
Compare
andreidan
left a comment
andreidan
left a comment
There was a problem hiding this comment.
Thanks for working on this Pablo.
Left some rather minor suggestions
| // The no-follower step is added here because an `unfollowAction` is added before the `shrinkAction` in the follower | ||
| // cluster. | ||
| stepRule(WaitForNoFollowersStep.NAME, ONE_DAY) | ||
| stepRuleOnlyCheckRetries(WaitForNoFollowersStep.NAME, MAX_RETRIES) |
There was a problem hiding this comment.
The comment can be removed - now that we're only looking at max retries for the WaitForNoFollowerStep.
(same for above in the SearchableSnapshotAction)
And thinking about it some more, I think it's ok to not have MAX_TIME on this step because we can have policies without rollover. So a policy with only the warm phase and a shrink action could stay in WaitForNoFollowerStep on the leader cluster for a long time if the index is actively written to still or if a user on the follower cluster doesn't manually (this is a policy without rollover - the rollover action sets this automatically otherwise) set the indexing_complete setting to true
| stepRule(WaitForIndexColorStep.NAME, ONE_DAY), | ||
| stepRuleFullChecks(WaitForDataTierStep.NAME, ONE_DAY, MAX_RETRIES), | ||
| stepRuleFullChecks(WaitForIndexColorStep.NAME, ONE_DAY, MAX_RETRIES), | ||
| // The no-follower step is added here because an `UnfollowAction` is added before the `shrinkAction` in the follower cluster |
There was a problem hiding this comment.
As bellow, we can remove this comment
| stepRuleFullChecks(ForceMergeStep.NAME, ONE_DAY, 100L), | ||
| stepRuleFullChecks(SegmentCountStep.NAME, ONE_DAY, 100L) |
There was a problem hiding this comment.
We could use MAX_RETRIES here
| public record StepRule(String step, TimeValue maxTimeOn, long maxRetries) implements RuleConfig { | ||
| static StepRule stepRule(String name, TimeValue maxTimeOn) { | ||
| return new StepRule(name, maxTimeOn, 100); | ||
| public record StepRule(String step, TimeValue maxTimeOn, Long maxRetries) implements RuleConfig { |
There was a problem hiding this comment.
Shall we override the canonical constructor and add a validation that either maxTimeOn or maxRetries are different than null ? And otherwise, if both are null, throw an IllegalArgumentException ?
3 participants
Allows having different StepRule shapes:
adding specific factory methods
Tackle PR #96092 's comments