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
Replace PhaseAfterStep with PhaseCompleteStep #33398
Replace PhaseAfterStep with PhaseCompleteStep #33398
Conversation
This removes `PhaseAfterStep` in favor of a new `PhaseCompleteStep`. This step in only a marker that the `LifecyclePolicyRunner` needs to halt until the time indicated for entering the next phase. This also fixes a bug where phase times were encapsulated into the policy instead of dynamically adjusting to policy changes. Supersedes elastic#33140, which it replaces
Pinging @elastic/es-core-infra |
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.
looks great! just left some q&c
x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java
Outdated
Show resolved
Hide resolved
- match: { indices.foo.index: "foo" } | ||
- match: { indices.foo.policy: "quick_warm_slow_cold" } | ||
- match: { indices.foo.phase: "warm" } | ||
- match: { indices.foo.action: "readonly" } |
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.
are there no potential timing issues here? Is the reason that this is relatively consistent because
the next time it'll proceed is from a scheduled poll, and that won't happen for another while?
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.
Yes, there are timing issues here, I'm going to remove this test
...ugin/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java
Show resolved
Hide resolved
...ugin/ilm/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java
Show resolved
Hide resolved
...ck/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java
Show resolved
Hide resolved
...ck/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java
Show resolved
Hide resolved
Thanks for the comments @talevy, I believe I addressed all your feedback |
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.
@dakrone I left a comment about the next step of the phase complete step, otherwise I think this change is good
public static final String NAME = "complete"; | ||
|
||
public PhaseCompleteStep(StepKey key, StepKey nextStepKey) { | ||
super(key, nextStepKey); |
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 think we need to make the next step key always null
in these steps? The reason is that the next step after this phase could change before we get there. I think we need to determine the next phase dynamically when we find ourselves at a PhaseCompleteStep
and have each phase separated from each other in the steps chain. I think this might mean having something like a MoveToPhaseUpdateTask
so these checks happen on the cluster state update thread where we know the cluster state is static and we can inspect the policy.
I am ok with this happening as a follow up PR though so this PR is kept more contained.
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.
@elasticmachine test this please |
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
…-phase-after-step
…-phase-after-step
This removes `PhaseAfterStep` in favor of a new `PhaseCompleteStep`. This step in only a marker that the `LifecyclePolicyRunner` needs to halt until the time indicated for entering the next phase. This also fixes a bug where phase times were encapsulated into the policy instead of dynamically adjusting to policy changes. Supersedes #33140, which it replaces Relates to #29823
This removes
PhaseAfterStep
in favor of a newPhaseCompleteStep
. This stepin only a marker that the
LifecyclePolicyRunner
needs to halt until the timeindicated for entering the next phase.
This also fixes a bug where phase times were encapsulated into the policy
instead of dynamically adjusting to policy changes.
Supersedes #33140, which it replaces
Relates to #29823