Refactor PhaseCacheManagement to apply refreshes in a bulk-safe manner #132660
Conversation
…bulk-safe manner.
jbaiera
added
>non-issue
:Data Management/ILM+SLM
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
jbaiera
changed the title
| try { | ||
| refreshPhaseDefinition(projectMetadataBuilder, index, newPolicy); | ||
| refreshedIndices.add(index.getIndex().getName()); | ||
| var idxBuilder = prepareRefreshPhaseDefinition(index, newPolicy); | ||
| refreshedIndices.add(idxBuilder); | ||
| } catch (Exception e) { | ||
| logger.warn(() -> format("[%s] unable to refresh phase definition for updated policy [%s]", index, newPolicy.getName()), e); | ||
| } |
There was a problem hiding this comment.
I am probably missing something. Since we catch exception with a warning log here and continue with the next index, wouldn't exception down the stack still result in partial update, seems we just delay putting the partial index metadata into the project metadata? I wonder if we should throw an exception instead?
There was a problem hiding this comment.
Though I haven't entirely gone through what's the implication on throwing here.
There was a problem hiding this comment.
The new logic batches up the list of changed index metadata objects and only adds them to the project metadata builder after collecting all of them. Overall, it's a very small refactoring, and practically speaking shouldn't modify any behavior. The bigger picture here is that I'm trying to make changes to the method contract to ensure that the project metadata builder passed is less likely to be modified unnecessarily since it may be carrying changes from another batch of cluster state operations.
There was a problem hiding this comment.
I am very much fine with the small refactoring (without behavior change). I probably misunderstood the PR description This saves from polluting a potentially shared project metadata builder instance with incomplete state updates in the event that an exception is thrown from lower down the call stack thinking it means "all or nothing" refresh on indices. But it just means that we don't update the project metadata until all indices are processed (with or without exception).
|
Actually, after some further thinking this evening, I think this refactoring might be redundant with some other changes I'm thinking of making for bulk metadata updates. As such, I'll just close this out. |
3 participants
Small refactoring which updates the PhaseCacheManagement logic to collect all of the phase refreshed index metadata before applying them to the project metadata builder all at once. This saves from polluting a potentially shared project metadata builder instance with incomplete state updates in the event that an exception is thrown from lower down the call stack.