[core][flink] Detect overwrite conflicts for index commits#7972
[core][flink] Detect overwrite conflicts for index commits#7972leaves12138 wants to merge 16 commits into
Conversation
8ddb1ea to
cd070af
Compare
cd070af to
a271615
Compare
| return rowIdCheckFromSnapshot != null; | ||
| } | ||
|
|
||
| public void setRowIdReassignCheckFromSnapshot(@Nullable Long rowIdReassignCheckFromSnapshot) { |
There was a problem hiding this comment.
Can we just reuse setRowIdCheckFromSnapshot?
There was a problem hiding this comment.
Thanks for the suggestion. I think these two snapshot markers should stay separate because they guard different invariants. rowIdCheckFromSnapshot is used by row-id range / column-level conflict checks, while this one is driven by commit.row-id-reassign.last-safe-snapshot and scans later OVERWRITE snapshots for row-id reassign or overlapped partition overwrite conflicts during index build/compact commits. Reusing the existing field would couple two independent checks and make the COMPACT/index path harder to reason about.
jerry-024
left a comment
There was a problem hiding this comment.
Overall LGTM. Three minor optimization suggestions below — none are blockers.
| Long rowIdReassignCheckFromSnapshot = | ||
| indexBuilder.scanSnapshotId().isPresent() | ||
| ? indexBuilder.scanSnapshotId().get() | ||
| : null; |
There was a problem hiding this comment.
nit: Can be simplified to:
Long rowIdReassignCheckFromSnapshot = indexBuilder.scanSnapshotId().orElse(null);Optional.orElse(null) is idiomatic and avoids the double method call.
| CoreOptions.COMMIT_ROW_ID_REASSIGN_LAST_SAFE_SNAPSHOT.key(), | ||
| String.valueOf(rowIdReassignCheckFromSnapshot))); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: This method is identical to BTreeIndexTopoBuilder.withRowIdReassignCheck. Consider extracting to a shared utility to avoid drift if the option key or logic changes later. Not blocking — the method is small.
| } | ||
|
|
||
| @Test | ||
| public void testRowIdReassignConflictFromOptions() throws Exception { |
There was a problem hiding this comment.
suggestion: Consider adding a test that verifies compactManifestOnce also strips ROW_ID_REASSIGN_PROPERTY. Current tests cover replaceManifestList but not the compact path.
fa46435 to
923532c
Compare
|
Reviewed commit 36f6f1a. The overwrite-conflict check looks scoped to index commits via the scan snapshot from the index builders, the overwrite barrier is treated as a one-shot marker and stripped from derived snapshots, and the added tests cover barrier, partition-overlap, non-overwrite, and data-only cases. I didn't find blocking issues from my side. |
JingsongLi
left a comment
There was a problem hiding this comment.
I cannot get why introducing OVERWRITE_BARRIER_PROPERTY. Can you explain more?
JingsongLi
left a comment
There was a problem hiding this comment.
After discussing, you should only need to check whether the RowID data exists
d244ec3 to
826a88f
Compare
Purpose
Prevent stale global index commits after later
OVERWRITEsnapshots change row-id/index safety.Changes
commit.overwrite-conflict-with-index.last-safe-snapshot.OVERWRITEsnapshots with transientoverwrite-barrier.Validation
git diff --checkmvn -pl paimon-api,paimon-docs -Pfast-build -DfailIfNoTests=false -Dtest=ConfigOptionsDocsCompletenessITCase#testCompleteness test