Simplify & consolidate SnapshotsService finalization #124537
Conversation
Splits up `SnapshotFinalization#doRun` into more focussed methods, brings all the other finalization-specific methods into the `SnapshotFinalization` inner class, and consolidate the exception handling.
DaveCTurner
added
:Distributed Coordination/Snapshot/Restore
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| SnapshotsInProgress.Entry entry = SnapshotsInProgress.get(clusterService.state()).snapshot(snapshot); | ||
| final String failure = entry.failure(); | ||
| logger.trace("[{}] finalizing snapshot in repository, state: [{}], failure[{}]", snapshot, entry.state(), failure); | ||
| final var entry = SnapshotsInProgress.get(clusterService.state()).snapshot(snapshot); |
There was a problem hiding this comment.
Commenting on the SnapshotFinalization declaration / class -- can't make a comment there
WDYT of following up on this patch to shove SnapshotFinalization into its own class file? Removing the snapshot/metadata/repositoryData objects from method parameters makes their use come out of thin air, sorta, and in the scope of this file, that makes it visually much less obvious where the values are coming from.
| handleFinalizationFailureBeforeUpdatingRootBlob(e); | ||
| } | ||
|
|
||
| private static ShardGenerations buildGenerations(SnapshotsInProgress.Entry snapshot, Metadata metadata) { |
There was a problem hiding this comment.
It's confusing that in some methods you're keeping the parameters and in others you remove them. Is this an oversight, or did you have a thought process in mind?
| public void onFailure(Exception e) { | ||
| logger.error(Strings.format("unexpected failure finalizing %s", snapshot), e); | ||
| assert false : new AssertionError("unexpected failure finalizing " + snapshot, e); | ||
| handleFinalizationFailureBeforeUpdatingRootBlob(e); |
There was a problem hiding this comment.
How do we know that this is always BEFORE updating the root blob?
Splits up
SnapshotFinalization#doRuninto more focussed methods,brings all the other finalization-specific methods into the
SnapshotFinalizationinner class, and consolidates the exceptionhandling.