Core: Load snapshot after it has been committed to prevent accidental cleanup of files#15511
Conversation
f768f5b to
6048f3b
Compare
|
|
||
| // loadTable is executed once | ||
| Mockito.verify(adapter) | ||
| Mockito.verify(adapter, times(2)) |
There was a problem hiding this comment.
The current loads are to refresh prior to commit and to refresh after commit?
There was a problem hiding this comment.
The current loads are to refresh prior to commit and to refresh after commit?
yes, exactly
| } | ||
|
|
||
| @TestTemplate | ||
| public void manifestNotCleanedUpWhenSnapshotNotLoadableAfterCommit() { |
There was a problem hiding this comment.
the test mainly hits the else branch in SnapshotProducer and thus skips manifest cleanup but I wasn't able to reproduce the theoretical issue that stuff is accidentally cleaned up and causes corrupt metadata so far
| taskOps -> { | ||
| Snapshot newSnapshot = apply(); | ||
| stagedSnapshot.set(newSnapshot); | ||
| newSnapshotId.set(newSnapshot.snapshotId()); |
There was a problem hiding this comment.
Do we need to set the snapshot ID here? It should be available everywhere using snapshotId().
There was a problem hiding this comment.
this is really just restoring the behavior to how it was prior to 39373d0 so I didn't do any other modifications to the code other than adding a test
| LOG.info("Committed snapshot {} ({})", newSnapshotId.get(), getClass().getSimpleName()); | ||
|
|
||
| // at this point, the commit must have succeeded. after a refresh, the snapshot is loaded by | ||
| // id in case another commit was added between this commit and the refresh. |
There was a problem hiding this comment.
Would be nice to mention the case we're fixing: that we may not know which attempt succeded in some cases and we will only clean up the one that actually did.
There was a problem hiding this comment.
makes sense, added some additional wording around this
| } | ||
| } else { | ||
| // saved may not be present if the latest metadata couldn't be loaded due to eventual | ||
| // consistency problems in refresh. in that case, don't clean up. |
There was a problem hiding this comment.
I don't know that I agree here. If we can't load the latest metadata, it should throw an exception. But anything could happen so unless we know exactly what was committed, it is safer to skip cleanup.
There was a problem hiding this comment.
3abb755 to
eb9d3df
Compare
|
thanks for reviewing @rdblue |
… cleanup of files (apache#15511)
… cleanup of files (apache#15511)
#10523 added an optimization where the snapshot isn't loaded from the latest table state after it has been committed. After talking to @danielcweeks @amogh-jahagirdar @rdblue we want to be more defensive when cleaning up files and we should be doing the cleanup based on the latest table state.
This means that we should be loading the snapshot after it has been been committed in order to prevent accidental cleanup of files that are based on a snapshot state that isn't necessarily the one that's committed