Skip to content
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

Remove canSetPolicy, canUpdatePolicy, and canRemovePolicy #33037

Merged
merged 3 commits into from Aug 23, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Aug 21, 2018

Since we now store a pre-compiled list of steps for an index's phase in the
PolicyStepsRegistry, we no longer need to worry about updating policies as any
updates won't affect the current phase, and will only be picked up on phase
transitions.

This also removes the tests that test these methods

Relates to #29823

Since we now store a pre-compiled list of steps for an index's phase in the
`PolicyStepsRegistry`, we no longer need to worry about updating policies as any
updates won't affect the current phase, and will only be picked up on phase
transitions.

This also removes the tests that test these methods

Relates to elastic#29823
@dakrone dakrone added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Aug 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@dakrone dakrone changed the title Remove canSetPolicy and canUpdatePolicy Remove canSetPolicy, canUpdatePolicy, and canRemovePolicy Aug 21, 2018
if (currentStepKey != null) {
// Check if current step exists in new policy and if not move to
// next available step
StepKey nextValidStepKey = newPolicy.getNextValidStep(currentStepKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thinks the getNextValidStep() stuff can go to no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I do that as a followup? I think this change ended up larger than I hoped due to having to change a lot of tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dakrone dakrone merged commit 52aa738 into elastic:index-lifecycle Aug 23, 2018
dakrone added a commit that referenced this pull request Aug 23, 2018
* Remove canSetPolicy, canUpdatePolicy and canRemovePolicy

Since we now store a pre-compiled list of steps for an index's phase in the
`PolicyStepsRegistry`, we no longer need to worry about updating policies as any
updates won't affect the current phase, and will only be picked up on phase
transitions.

This also removes the tests that test these methods

Relates to #29823
@dakrone dakrone mentioned this pull request Aug 24, 2018
@dakrone dakrone deleted the ilm-remove-canSetPolicy branch February 4, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants