Remove PhaseAfterStep #33140
Remove PhaseAfterStep #33140
Conversation
This commit removes PhaseAfterStep and all the plumbing associated with it. Instead, we rely on the LifecyclePolicyRunner to police itself for advancing phases. This also makes a modification to the settings that are exposed related to the current phase, instead of returning the current phase/step/action as-is in the `index.lifecycle.phase` (etc) setting, these are now split into: `index.lifecycle.current_phase|action|step` - the currently executing phase/action/step which may or may not have completed `index.lifecycle.next_phase|action|step` - the next phase/action/step to which we will be proceeding While I don't think these will cause much issue (especially since nothing is being broken for users here), these changes were required to have the `phase_time` correctly updated now that we don't have a "shim" step between phases. Without these it would be confusing as the index would advance to have an `index.lifecycle.phase` setting that was potentially one phase in the future. Relates to elastic#29823
dakrone
added
the
:Data Management/ILM+SLM
| } | ||
| } | ||
|
|
||
| private StepKey getNextAfterStep(String currentPhaseName) { |
There was a problem hiding this comment.
This is the removal of these as a followup to #33037 (comment)
talevy
left a comment
talevy
left a comment
There was a problem hiding this comment.
looks good, I just left some clarifying questions
| } | ||
| } | ||
|
|
||
| private StepKey getNextAfterStep(String currentPhaseName) { |
| // Before executing this step, we need to check whether the index is | ||
| // ready to transition to whatever phase this step is in. If it's | ||
| // not, then we need to return and wait until the future when it will be | ||
| if (isReadyToTransitionToThisPhase(policy, indexMetaData, currentStep.getKey().getPhase()) == false) { |
There was a problem hiding this comment.
should this check be in the same place where we make the actual transition? then it doesn't look a LIFECYCLE_FORCED_PHASE would have to be set as a signal back to this execution.
There was a problem hiding this comment.
It can't be moved there, because otherwise we would re-execute the current steps' actions until the next phase was ready
| return firstStepMap.get(policy); | ||
| } | ||
|
|
||
| public TimeValue getIndexAgeForPhase(final String policy, final String phase) { |
There was a problem hiding this comment.
should we keep the after naming here until it is actually settled?
There was a problem hiding this comment.
I think calling it getAfterForPhase is more confusing than getIndexAgeForPhase, so I'd like to keep it regardless of what we decide on naming
| String currentPhase = LifecycleSettings.LIFECYCLE_PHASE_SETTING.get(indexSettings); | ||
| String currentAction = LifecycleSettings.LIFECYCLE_ACTION_SETTING.get(indexSettings); | ||
| String currentStep = LifecycleSettings.LIFECYCLE_STEP_SETTING.get(indexSettings); | ||
| String currentPhase = LifecycleSettings.LIFECYCLE_NEXT_PHASE_SETTING.get(indexSettings); |
There was a problem hiding this comment.
is it the intention to have getCurrentStepKey return the "NEXT_SETTING", while there exists a "CURRENT_SETTING" that can be misunderstood to be just that, the current setting? seems like it is more a "previous" setting
There was a problem hiding this comment.
It technically is a "previous" setting from the viewpoint of the internal code, but from an external view (a user looking at the explain or get-settings output) it's the current step, because the age has not yet advanced enough for it
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
|
Closed in favor of #33398 |
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 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
2 participants
This commit removes PhaseAfterStep and all the plumbing associated with it.
Instead, we rely on the LifecyclePolicyRunner to police itself for advancing
phases.
This also makes a modification to the settings that are exposed related to the
current phase, instead of returning the current phase/step/action as-is in the
index.lifecycle.phase(etc) setting, these are now split into:index.lifecycle.current_phase|action|step- the currently executingphase/action/step which may or may not have completed
index.lifecycle.next_phase|action|step- the next phase/action/step to whichwe will be proceeding
While I don't think these will cause much issue (especially since nothing is
being broken for users here), these changes were required to have the
phase_timecorrectly updated now that we don't have a "shim" step betweenphases. Without these it would be confusing as the index would advance to have
an
index.lifecycle.phasesetting that was potentially one phase in the future.Relates to #29823