Fix PolicyStepsRegistry's cachedSteps null handling #84588
Conversation
Since we're invoking getKey on the cached step (see cachedStep.v2().getKey() above), the cached step is never allowed to be null.
To make sure it always works as expected
joegallo
added
>bug
:Data Management/ILM+SLM
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @joegallo, I've created a changelog YAML for you. |
| cachedSteps.put(indexMetadata.getIndex(), Tuple.tuple(indexMetadata, s)); | ||
| // assert that the cache works as expected -- that is, if we put something into the cache, | ||
| // we should get back the same thing if we were to invoke getStep again with the same arguments | ||
| assert s == getStep(indexMetadata, stepKey); |
There was a problem hiding this comment.
I don't think I like this assert, because if the cache were to fail (let's say someone comes along and changes some of the if statements above), then it could end up recursing until a stack overflow. I don't think we need this assert
There was a problem hiding this comment.
What do you think about 5b9b84c? It makes it non-recursive, so there's no risk of stackoverflow, and also getCachedStep is more clearly read-only w.r.t. the cachedSteps map so this should never modify state.
There was a problem hiding this comment.
That looks better to me, no recursion and no chance to modify state in a assert. Thanks!
x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java
Outdated
Show resolved
Hide resolved
Fix for a bug introduced in #82316
In the event that we cache a null (~= "not found") for a given IndexMetadata and StepKey, then subsequent lookups via that same IndexMetadata and StepKey will NPE instead of merely returning null.
As a fix, I've gone with no longer caching nulls paired with asserts that make sure we never find a null in the cache, and that if we put something into the cache we can always pull that exact same thing back out on a subsequent call to
getStepgetCachedStep with the same arguments.