Skips to next available action on missing step #32283
Conversation
colings86
added
review
:Data Management/ILM+SLM
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
should we find a way to do this so that either toSteps is derived from toStepKeys, or vice-versa? The separation of these in ways that need to be kept consistent sounds like it may be difficult to keep in sync
There was a problem hiding this comment.
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 toStepKeys and 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 of toSteps meant passing in a null client 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 methods AbstractActionTestCase has a method which create a random instance of the action under test and makes sure that the step keys from the steps generated by toSteps() are the same as the step keys created by toStepKeys(): https://github.com/elastic/elasticsearch/pull/32283/files#diff-99c08c350d9cf07e3a5366ab534f80f7R28
There was a problem hiding this comment.
OK, then I guess that is the best we can do for now. I do not see a better way at the moment either
|
I think the CI failure is just because this needs latest merged in |
There was a problem hiding this comment.
I'm not sure I follow why the step before a phase would be the terminal step?
There was a problem hiding this comment.
oh, I think I get it. you are looking for the after-step for the next phase, but there is no next phase, so we go straight to terminalpolicystep. If this is the case, some extra docs would help make that clearer
There was a problem hiding this comment.
Yes you are correct, I'll try to clarify it better int eh comment
There was a problem hiding this comment.
can we add javadocs to guide the comments in the branches?
|
@talevy I added the comments and java docs you asked for. Could you look again? |
talevy
left a comment
talevy
left a comment
There was a problem hiding this comment.
LGTM with the NORELEASE follow-up in mind
if policy update on index means current step no longer exists This change only updates the setPolicy for index to add this functionality. The update policy API will be changed in a follow up PR.
if policy update on index means current step no longer exists This change only updates the setPolicy for index to add this functionality. The update policy API will be changed in a follow up PR.
3 participants
if policy update on index means current step no longer exists
This change only updates the setPolicy for index to add this
functionality. The update policy API will be changed in a follow up PR.