Rebuild step on PolicyStepsRegistry.getStep#33780
Rebuild step on PolicyStepsRegistry.getStep#33780dakrone merged 3 commits intoelastic:index-lifecyclefrom
Conversation
This moves away from caching a list of steps for a current phase, instead rebuilding the necessary step from the phase JSON stored in the index's metadata. Relates to elastic#29823
dakrone
added
the
:Data Management/ILM+SLM
| } | ||
|
|
||
| static Step getCurrentStep(PolicyStepsRegistry stepRegistry, String policy, Index index, Settings indexSettings) { | ||
| static Step getCurrentStep(PolicyStepsRegistry stepRegistry, String policy, IndexMetaData indexMetaData, Settings indexSettings) { |
There was a problem hiding this comment.
This is going to cause a (not very big, I think?) merge conflict with #33783. Not sure which side that should be handled on, but fair warning.
| indexPhaseSteps.get(index).stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null)); | ||
|
|
||
| // parse phase steps from the phase definition in the index settings | ||
| final String phaseJson = indexMetaData.getSettings().get(LifecycleSettings.LIFECYCLE_PHASE_DEFINITION, |
There was a problem hiding this comment.
This will also need to be updated for #33783, depending on which gets merged first.
bleskes
left a comment
bleskes
left a comment
There was a problem hiding this comment.
LGTM, as a first start. I think we should follow up with removing PolicyStepsRegistry completely. There is no reason for it to exist IMO. Let me know if I miss something.
| default: | ||
| throw new IllegalArgumentException("invalid action [" + action + "]"); | ||
| }}; | ||
| for (String phase : phaseNames) { |
There was a problem hiding this comment.
maybe we're not ready for this yet but don't we want a random subset of the phases and actions?
There was a problem hiding this comment.
For now this made it easier to pick a random step for the entire policy because it guaranteed that I could pick a random phase that would have a random step. For this particular test it also tests (through randomization) that all phases and steps can be serialized/deserialized to/from JSON in the metadata
| static Step getCurrentStep(PolicyStepsRegistry stepRegistry, String policy, IndexMetaData indexMetaData, Settings indexSettings) { | ||
| StepKey currentStepKey = getCurrentStepKey(indexSettings); | ||
| if (currentStepKey == null) { | ||
| return stepRegistry.getFirstStep(policy); |
There was a problem hiding this comment.
As far as I can tell, this only needed to onboard an index. Can we move this (as a follow up) to a dedicated place? on boarding is an exceptional case - we should put it in it's own method.
There was a problem hiding this comment.
Yeah as a followup definitely, we'd need to remove it if we want to remove PolicyStepsRegistry too
This moves away from caching a list of steps for a current phase, instead rebuilding the necessary step from the phase JSON stored in the index's metadata. Relates to #29823
3 participants
This moves away from caching a list of steps for a current phase, instead
rebuilding the necessary step from the phase JSON stored in the index's
metadata.
Relates to #29823