-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Skips to next available action on missing step #32283
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
Changes from all commits
d54012a
c1eadc2
21405b9
a4cba15
47986f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,6 +245,82 @@ public boolean isActionSafe(StepKey stepKey) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Finds the next valid {@link StepKey} on or after the provided | ||
| * {@link StepKey}. If the provided {@link StepKey} is valid in this policy | ||
| * it will be returned. If its not valid the next available {@link StepKey} | ||
| * will be returned. | ||
| */ | ||
| public StepKey getNextValidStep(StepKey stepKey) { | ||
| Phase phase = phases.get(stepKey.getPhase()); | ||
| if (phase == null) { | ||
| // Phase doesn't exist so find the after step for the previous | ||
| // available phase | ||
| return getAfterStepBeforePhase(stepKey.getPhase()); | ||
| } else { | ||
| // Phase exists so check if the action exists | ||
| LifecycleAction action = phase.getActions().get(stepKey.getAction()); | ||
| if (action == null) { | ||
| // if action doesn't exist find the first step in the next | ||
| // available action | ||
| return getFirstStepInNextAction(stepKey.getAction(), phase); | ||
| } else { | ||
| // if the action exists check if the step itself exists | ||
| if (action.toStepKeys(phase.getName()).contains(stepKey)) { | ||
| // stepKey is valid still so return it | ||
| return stepKey; | ||
| } else { | ||
| // stepKey no longer exists in the action so we need to move | ||
| // to the first step in the next action since skipping steps | ||
| // in an action is not safe | ||
| return getFirstStepInNextAction(stepKey.getAction(), phase); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private StepKey getNextAfterStep(String currentPhaseName) { | ||
| String nextPhaseName = type.getNextPhaseName(currentPhaseName, phases); | ||
| if (nextPhaseName == null) { | ||
| // We don't have a next phase after this one so there is no after | ||
| // step to move to. Instead we need to go to the terminal step as | ||
| // there are no more steps we should execute | ||
| return TerminalPolicyStep.KEY; | ||
| } else { | ||
| return new StepKey(currentPhaseName, PhaseAfterStep.NAME, PhaseAfterStep.NAME); | ||
| } | ||
| } | ||
|
|
||
| private StepKey getAfterStepBeforePhase(String currentPhaseName) { | ||
| String nextPhaseName = type.getNextPhaseName(currentPhaseName, phases); | ||
| if (nextPhaseName == null) { | ||
| // We don't have a next phase after this one so the next step is the | ||
| // TerminalPolicyStep | ||
| return TerminalPolicyStep.KEY; | ||
|
||
| } else { | ||
| String prevPhaseName = type.getPreviousPhaseName(currentPhaseName, phases); | ||
| if (prevPhaseName == null) { | ||
| // no previous phase available so go to the | ||
| // InitializePolicyContextStep | ||
| return InitializePolicyContextStep.KEY; | ||
| } | ||
| return new StepKey(prevPhaseName, PhaseAfterStep.NAME, PhaseAfterStep.NAME); | ||
| } | ||
| } | ||
|
|
||
| private StepKey getFirstStepInNextAction(String currentActionName, Phase phase) { | ||
| String nextActionName = type.getNextActionName(currentActionName, phase); | ||
| if (nextActionName == null) { | ||
| // The current action is the last in this phase so we need to find | ||
| // the next after step | ||
| return getNextAfterStep(phase.getName()); | ||
| } else { | ||
| LifecycleAction nextAction = phase.getActions().get(nextActionName); | ||
| // Return the first stepKey for nextAction | ||
| return nextAction.toStepKeys(phase.getName()).get(0); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(name, phases); | ||
|
|
||
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.
should we find a way to do this so that either
toStepsis derived fromtoStepKeys, or vice-versa? The separation of these in ways that need to be kept consistent sounds like it may be difficult to keep in syncThere 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.
Yeah I thought about this at the time but the problem is that in order to derive the steps from the step keys we would end up having some ugly if-else logic to iterate through the keys returned by
toStepKeysand for each try to work out which step it is based on and build that step. The reverse of building the stepKeys based on the output oftoStepsmeant passing in anullclient in production code which I also didn't like because it could potentially cause NPE's. In order to try to keep consistency between the two methodsAbstractActionTestCasehas a method which create a random instance of the action under test and makes sure that the step keys from the steps generated bytoSteps()are the same as the step keys created bytoStepKeys(): https://github.com/elastic/elasticsearch/pull/32283/files#diff-99c08c350d9cf07e3a5366ab534f80f7R28There 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.
OK, then I guess that is the best we can do for now. I do not see a better way at the moment either