Repository navigation
[core] Keep main's snapshots when fast_forward fails and hold the commit lock - #10845
LuciferYang wants to merge 4 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.
There was a problem hiding this comment.
The source-read case and the same-length intermediate-snapshot case now pass at 02d7d676dc. One part of this lease boundary is still reproducible: copyAtomically checks the lease before the separate fileIO.exists(target) call. That remote metadata request can outlast the lease, then its false result leads to tryToWriteAtomic without another check.
I ran actual table.fastForward("b1") with main snapshot 1 and branch snapshot 2. The test FileIO expires the lease while checking whether main snapshot 2 exists, returns false, and then allows the normal write. Fast-forward eventually throws Lease expired, but main snapshot 2 already exists. The assertion that snapshot 2 remains absent fails on this head. With only an additional ensureValid() after the existence query and before the atomic write, the identical test passes.
Please include the target existence query among preparation performed before the final lease check. The 66 existing Core cases pass normal JDK 8 Maven verify; this extra timing case is not covered by those tests. Temporary reviewer changes were removed.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in baef7eb. copyAtomically now checks whether the target exists before the final lease check, so nothing remote runs between that check and the write. Hint writes go through a commitHint overload that checks the lease before every attempt, including retries. testFastForwardRechecksLeaseAfterCheckingTarget expires the lease during exists on main's snapshot 2 and asserts it stays absent; testFastForwardRechecksLeaseBeforeRetryingHint covers the retry.
| 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
|
Could you please take another look when you have time? @JingsongLi Thanks ~ |
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 and checking whether the target exists) and before each attempt to write a hint file, 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,testFastForwardRechecksLeaseAfterCheckingTarget,testReplaceStopsDeletingWhenLeaseExpires,testFastForwardRechecksLeaseBeforeRetryingHint,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, while the target is checked, between deletes, or before a failed hint write is retried 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