Replace PhaseAfterStep with PhaseCompleteStep#33398
Replace PhaseAfterStep with PhaseCompleteStep#33398dakrone merged 8 commits intoelastic:index-lifecyclefrom
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 |
talevy
left a comment
There was a problem hiding this comment.
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.
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.
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 |
| public static final String NAME = "complete"; | ||
|
|
||
| public PhaseCompleteStep(StepKey key, StepKey nextStepKey) { | ||
| super(key, nextStepKey); |
There was a problem hiding this comment.
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.
|
@elasticmachine test this please |
…-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
PhaseAfterStepin favor of a newPhaseCompleteStep. This stepin only a marker that the
LifecyclePolicyRunnerneeds 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