Repository navigation
[core] Keep main's snapshots when fast_forward fails and hold the commit lock - #10845
LuciferYang wants to merge 2 commits into
Conversation
|
cc @JingsongLi |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
Protecting fast-forward against interrupted copies and concurrent commits has clear production value. Normal JDK 8 Maven verify passes 61 unique Core cases (FileSystemBranchManagerTest and CatalogEnvironmentTest), and this head's CI is green. Two additional real-table fault-injection cases expose gaps below: publication after lease loss during source IO, and an equal-length divergent intermediate snapshot left unrepaired after an exact baseline interrupted replacement. Both have negative controls; repository sources were left unchanged.
| // A lease may expire while many files are copied; stop before writing past it. | ||
| lock.ensureValid(); | ||
| String content = readExactly(source); | ||
| if ((fileIO.exists(target) || !fileIO.tryToWriteAtomic(target, content)) |
There was a problem hiding this comment.
[P2] Revalidate the lease after source IO and before publication
The lease check occurs before readExactly(source), so a remote read can outlast the lease or a failed heartbeat. I reproduced this through actual table.fastForward: main has snapshot 1, a tag-derived branch has snapshot 2, and FileIO expires the lease while opening branch snapshot 2. tryToWriteAtomic still publishes main snapshot 2 with expired=true; fastForward only throws at a later check. An identical control with ensureValid() immediately after the source read leaves main snapshot 2 absent. Please recheck after potentially blocking preparation and before the actual mutation, and cover this timing rather than only expiry between completed writes. This is a gap in the newly promised lease protection (the baseline did not provide locking). Apply the same boundary discipline to hint writes and multi-file deletes.
There was a problem hiding this comment.
Fixed in 02d7d67. copyAtomically now checks the lease after reading the source, right before the write. Replacing main deletes one file at a time with a check before each delete, and the LATEST and EARLIEST hint writes are checked too. testFastForwardRechecksLeaseAfterReadingSource expires the lease while the branch's snapshot 2 is opened and asserts main gets no snapshot 2; testReplaceStopsDeletingWhenLeaseExpires expires it after the first delete and asserts nothing else is deleted.
| Map<Long, Long> sizes, Map<Long, Long> branchSizes, long from, long to) { | ||
| for (long id = Math.max(from, Collections.min(sizes.keySet())); id <= to; id++) { | ||
| Long size = sizes.get(id); | ||
| if (size == null || !size.equals(branchSizes.get(id))) { |
There was a problem hiding this comment.
[P2] Verify intermediate snapshot content before treating main as a prefix
Equal size does not establish snapshot identity. With real writes, divergent main and branch snapshot 3 are both 639 bytes. I executed the exact baseline fastForward with failed quiet metadata deletions, listing snapshot 4 before snapshot 3, and an injected failure before overwriting snapshot 3. This leaves branch snapshot 4 on main, the divergent full-length main snapshot 3, and matching schema files. After disabling all faults, this head's fastForward returns success because only the latest snapshot is compared by content. A time-travel read of main snapshot 3 still returns rows [1,2,5], while branch snapshot 3 returns [1,2,3]. Removing main snapshot 3 to force replacement repairs the same table. Please establish content identity for every retained snapshot in the claimed prefix, or use another reliable lineage/integrity check; length is insufficient for the interrupted-old-replace recovery this PR supports.
There was a problem hiding this comment.
Fixed in 02d7d67. The prefix check now compares the content of every main snapshot from the branch's earliest (or main's earliest, if later) to main's latest with the branch's, bytes first and parsed snapshots if the bytes differ. testFastForwardReplacesDivergentSnapshotOfSameLength builds the state you describe: main's own snapshot 3, the same length as the branch's, next to the branch's snapshot 4. fast_forward now replaces main, and snapshot 3 ends up with the branch's bytes.
Replacing main also no longer deletes quietly. A file that is still there after the delete fails the fast-forward (testReplaceFailsWhenSnapshotCannotBeDeleted), so this version does not leave that state behind itself.
…pare the whole prefix
Purpose
On a filesystem-managed table,
fast_forwarddeletes main's snapshots, schemas and tags from the branch's earliest snapshot onwards together with the LATEST hint, then copies the branch's snapshot, schema and tag directories in listing order. It does this without the lock that commits use. A failure part way leaves main without the snapshot the branch was created from, with missing or truncated snapshot files, and without the schemas its remaining snapshots use. A commit to main can also interleave with the delete and copy.With this change:
fast_forwardholds the lock that commits to main use for the whole operation, and checks that the lock is still valid right before each file it writes or deletes (after reading the file it copies), so it stops once a lease-based lock such as the Hive lock has expired. Without a configured lock this is a no-op, as for commits.tryToWriteAtomic, the tags that differ, and the LATEST hint. Nothing on main is deleted, so after a failure main ends at one of the branch's snapshots and runningfast_forwardagain continues from there. Main's latest snapshot is found by listing, not from the LATEST hint.tryToWriteAtomic. Main's tags are deleted before its snapshots, and a main snapshot, schema or tag that cannot be deleted fails the fast-forward instead of being left behind.The states that an interrupted
fast_forwardof an earlier version leaves (a LATEST hint naming a missing snapshot, a gap, a truncated snapshot, main's own snapshot left next to the branch's) fail the prefix check, so the nextfast_forwardreplaces main and repairs them.Publication is still not atomic. Readers can see some of the branch's snapshots before the rest are copied, and when the branch has newer schemas, main's latest schema switches to the branch's before its snapshots arrive.
Other differences from the current behavior: tags on main that point at snapshots it shares with the branch are kept instead of deleted and copied back, and main's EARLIEST hint is no longer overwritten with the branch's. Replacing main moves it back to the branch's earliest snapshot only when main had expired past it.
Tables whose catalog manages versions, such as REST, use
CatalogBranchManagerand are not changed.Related to #9690.
Tests
New cases in
FileSystemBranchManagerTest, with a test FileIO that lists files in name order like HDFS and object stores do, and can fail a chosen write:testFailedFastForwardKeepsMainSnapshots,testFastForwardResumesAfterPartialFailure: a snapshot write into main fails. Main stays readable at snapshot 2 or 3, and the retry writes only the missing snapshots and deletes nothing.testFastForwardStopsWhenLockLeaseExpires,testFastForwardRechecksLeaseAfterReadingSource,testReplaceStopsDeletingWhenLeaseExpires,testFastForwardRunsUnderCommitLock: on both paths, every change to main's snapshot files happens while the lock is held, on the same identifier as main's commits, and a lease that expires between writes, while a source file is read, or between deletes stops the next change.testReplaceFailsWhenSnapshotCannotBeDeleted,testReplaceDeletesTagsBeforeSnapshots: a main snapshot that cannot be deleted fails the fast-forward. The retry leaves main at the branch's latest, and does not keep a tag on main's own snapshot when it appends to what the failed attempt left.testFastForwardRepairsInterruptedReplace,testFastForwardReplacesDivergentSnapshotOfSameLength: four states an interrupted earlier version can leave in main's snapshot directory are repaired to the branch's bytes, and so is main's own snapshot 3 left next to the branch's snapshot 4 with the same file length.testFailedReplaceKeepsMainSchemas,testFailedSchemaReplaceKeepsMainReadable: a snapshot or schema write fails while main is replaced, and the snapshot main keeps can still be read.testFastForwardCopiesSchemasInIdOrder,testFastForwardCopiesBranchSchemas,testFastForwardAfterMainSchemaChangeReplacesMain: schemas 2 to 10 are written in id order; a column added on the branch reaches main; when main and the branch both created schema 2, main ends with the branch's.testFastForwardAfterMainDivergedReplacesMain,testFastForwardKeepsMainTagsUnchanged: main's own snapshot 3 is replaced by the branch's; a tag main shares with the branch is not rewritten.testFastForwardAppendsAfterMainExpiredSharedSnapshots,testReplaceMovesEarliestHintBack: after main expires the snapshots it shares with the branch, the nextfast_forwardstill only appends; replacing a main that expired past the branch's earliest snapshot moves EARLIEST back, also when the first attempt fails and the retry appends.API and Format
no
Documentation
no