Provide useful error when a policy doesn't exist #34206
Conversation
When an index is configured to use a lifecycle policy that does not exist, this will now be noted in the step_info for that policy.
gwbrown
added
the
:Data Management/ILM+SLM
|
Pinging @elastic/es-core-infra |
dakrone
left a comment
dakrone
left a comment
There was a problem hiding this comment.
I left a couple of comments and a question
| + "] with policy [" + policy + "] is not recognized"); | ||
| return; | ||
| if (stepRegistry.policyExists(policy) == false) { | ||
| logger.trace("policy [{}] for index [{}] does not exist, recording this in step_info for this index", |
There was a problem hiding this comment.
I think this should be at debug level
| new StepInfoExceptionWrapper(new IllegalArgumentException("policy [" + policy + "] does not exist"))); | ||
| return; | ||
| } else { | ||
| logger.error("current step [" + getCurrentStepKey(lifecycleState) + "] for index [" + indexMetaData.getIndex().getName() |
There was a problem hiding this comment.
Can you change this to use parameterized logging?
| if (o == null || getClass() != o.getClass()) return false; | ||
| StepInfoExceptionWrapper that = (StepInfoExceptionWrapper) o; | ||
| return Objects.equals(exception.getMessage(), that.exception.getMessage()) | ||
| && Objects.equals(exception.getClass(), that.exception.getClass()); |
There was a problem hiding this comment.
Is there a reason we shouldn't do Objects.equals(exception, that.exception)? Do we want to have exceptions with different stacktraces be considered the same?
There was a problem hiding this comment.
I went back and forth on this too, but there's two things:
- This is for serializing to
step_info, and only the exception type and message get into the serialized version, so you can argue that it makes sense to check those here. - If we just check equality of the exception here, then do you know of a good way to get this test working?
I'd be delighted to go with a different approach here, I don't really like this class if I'm being honest. Do you have a recommendation for a different way to approach this?
There was a problem hiding this comment.
One option might be for this class to hold the already serialised form of the exception, but I'm not sure if that is better or worse than the current solution
|
I've addressed most of the comments and will explore some alternative solutions to replace |
When an index is configured to use a lifecycle policy that does not exist, this will now be noted in the step_info for that policy.
4 participants
When an index is configured to use a lifecycle policy that does not
exist, this will now be noted in the step_info for that policy.
Contrary to what's noted in the associated issue (#33074), this does
not move the index to the Error step, and instead sets the
step_infowith a note that the policy does not exist. This prevents issues with
recovering from this state.
Resolves #33074