Repository navigation
Conversation
9489c7f to
adca5e5
Compare
|
@JingsongLi can I get review on the PR? |
c5bd3d4 to
31cf7f2
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
Verified the user-facing late-delete problem in #11019. On 31cf7f2cd, 289 selected local Core tests passed and the exact-head Java CI matrix was green. The inline comments include three additional real-table reproductions outside the submitted suites.
| "delete-ordering-guards.max-sequence-number"; | ||
|
|
||
| /** Conservative hint, retained through resets along with the internal sequence watermark. */ | ||
| public static boolean mayContainGuards(@Nullable Snapshot snapshot) { |
There was a problem hiding this comment.
[P1] Prevent unsupported clients from exposing guarded rows or dropping guard discovery
I reproduced this with one filesystem PK table and the Java and PyPaimon clients from 31cf7f2cd. Java compacted a delete for key 1 at version 4, then appended version 2. Java returned no rows. The normal PyPaimon scan returned the stale row. I then appended an unrelated key from Python. The new snapshot kept the same index manifest and lost the guard properties, so Java now returned the stale row too. This property alone gates discovery of the still-present guard. Please reject guarded table reads/writes in unsupported clients before they can expose data or publish metadata, and account for older clients during rollout.
There was a problem hiding this comment.
Added PyPaimon validation before planning, reading, writer/backend initialization, and commit publication, including retries. It rejects both capture-enabled tables and retained guard history after capture is disabled. Regression coverage includes rejection before native backend selection and rejection of staged commits. The documentation also requires upgrading Java clients, coordinators, and workers together and stopping older clients before enabling capture.
| entry.kind() == FileKind.DELETE | ||
| && DELETE_ORDERING_GUARD.equals( | ||
| entry.indexFile().indexType())); | ||
| boolean publishesGuards = |
There was a problem hiding this comment.
[P1] Fence compactions built without a newly committed guard
Using real writers with both lookup and full-compaction changelog producers, I opened writer B and buffered key 1/version 2. Writer A then deleted version 4, fully compacted and published a guard-only snapshot. B compacted the buffered version 2 with its older view and committed successfully. A native stream resumed after A's snapshot emitted +I(1,2), while a batch scan of the same table returned no rows. This validation checks active guards only when the commit retires guards. Please validate the guard source set used to build data/changelog output for the affected bucket, and refresh it before retrying a changed set.
There was a problem hiding this comment.
Each compaction publication attempt now validates the writer’s observed guard files against the current committed set for the affected partition and bucket, including publication retries. A changed set rejects the commit and requires reopening the writer and recomputing compaction/changelog output. Added real-writer regressions for lookup and full-compaction changelogs, a publication-retry race, and an unrelated-partition control.
| SequenceSnapshotProperties.mergeGuardMaxSequenceNumber( | ||
| targetSnapshot.properties(), latest, deltaFiles, false); | ||
| rollbackProperties = | ||
| DeleteOrderingGuardGeneration.merge( |
There was a problem hiding this comment.
[P1] Keep a destructive rollback from reviving fenced writers
On a guarded fixed-bucket table I committed key 1, staged key 2 under generation 0, and truncated. The staged commit was rejected. Then I used the public table.rollbackTo(snapshot1) API and retried the exact staged message. It succeeded and Java read [1,2]. The ordinary rollback path used by sys.rollback_to restores the old snapshot generation, while the generation change here only covers rollback-as-latest. Please fence the public path with state that survives a head rewind, or reject destructive rollback on guarded histories and route users to an atomic as-latest reset.
There was a problem hiding this comment.
Destructive snapshot/tag rollback now rejects histories with capture enabled or tracked guard history, including after capture is disabled or the table is truncated. The check also handles a latest hint pointing to a missing snapshot. PyPaimon rejects before catalog delegation or local cleanup. rollbackToAsLatest remains the supported restoration path. Regression tests verify that the exact staged committable remains fenced.
|
|
||
| @Override | ||
| public MergeFileSplitRead newRead() { | ||
| validateDeleteOrderingGuardReadScope(); |
There was a problem hiding this comment.
[P2] Avoid loading the current snapshot per reader on guard-disabled tables
This runs before the reader sees its split. validateDeleteOrderingGuardReadScope() unconditionally fetches latestSnapshot, even for an ordinary PK table and even if the split is pinned to an older snapshot. Spark constructs a reader per partition, so a REST-backed query adds a current-snapshot request for each task instead of using the coordinator's planned metadata. A live catalog failure can also block a read whose pinned split and files are already available. Could this check use the planned split's guard metadata and stay out of the default-off reader initialization path?
There was a problem hiding this comment.
Removed the current-snapshot validation from reader initialization. Readers now validate guard support using metadata attached to the planned split. Added a pinned-split regression with a snapshot loader that throws, verifying that ordinary and guarded reads never call it. Raw readers also reject guarded splits using their planned metadata.
JingsongLi
left a comment
There was a problem hiding this comment.
Additional release rollback check using a real two-message committable.
| @@ -53,7 +53,7 @@ | |||
| /** {@link VersionedSerializer} for {@link CommitMessage}. */ | |||
| public class CommitMessageSerializer implements VersionedSerializer<CommitMessage> { | |||
|
|
|||
There was a problem hiding this comment.
[P2] Preserve rollback compatibility for checkpoints with ordinary commit messages
I staged two partition commit messages on an ordinary table with guards disabled. The new serializer wrote both with inner version 15 and a trailing generation-presence byte per message inside ManifestCommittable version 5. The new reader decoded both. The previous version 14 reader accepted the outer envelope, left the new byte unread, then failed on the next message with BufferUnderflowException. This affects the savepoint produced by an upgraded job even when it never uses guards. Could the ordinary message list retain the old encoding when no generation is present, or make the rollback restriction explicit and reject the unsupported envelope clearly?
There was a problem hiding this comment.
Took the explicit-rejection option suggested here. The manifest checkpoint envelope is now version 6, and the affected Flink envelopes are versioned so older readers reject the unsupported format before parsing the changed message layout. The documentation explicitly states that downgrading newly written checkpoints/savepoints is unsupported even with guard capture disabled, and recommends retaining a pre-upgrade savepoint with its matching table recovery point. Added two-message ordinary-committable coverage and retained supported legacy-format checks.
31cf7f2 to
ef72277
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS. Retaining payload-free ordering bounds addresses the verified late-insert resurrection problem for the documented fixed-bucket deduplicate scope.
Reviewed all 107 files with separate commit/serialization, reader/client-boundary, maintenance/policy and storage/compaction lenses. Previous Python client fencing, stale compaction source-set validation, destructive rollback rejection, default-off read initialization and explicit checkpoint downgrade restrictions were rechecked. Three additional real-table failures are detailed inline and were independently rerun.
Local validation: normal JDK8 verify passed 300 guard/schema/compatibility Core tests, 32 existing default compaction/changelog regressions and 3 Flink serializer/rescale tests; 249 Python tests passed. Spark3.5 normal verify passed 12 validation/rescale SQL tests, including actual delete/compaction and capture-disabled retained-guard rejection. Focused probes fail for branch promotion, expired-predecessor delta planning and rollback across a documented ordering-policy reset, while the unguarded delta control passes. No production source edits. Current-head CI run42863328579 has Python failures, so the older green run cited in the description is not a release result for this head. Coordinators/workers/clients must upgrade together; new-format checkpoint downgrade remains unsupported as documented. Release gate: BLOCK pending the confirmed failures.
| if (coreOptions().deleteOrderingGuardsEnabled() | ||
| || DeleteOrderingGuardGeneration.isTracked(latest) | ||
| || DeleteOrderingGuardFile.mayContainGuards(latest)) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
[P1] Apply destructive-history fencing to fastForward as well
The new rejection protects rollbackTo, but public fastForward still copies an old branch's generation onto main through FileSystemBranchManager. I reproduced this sequentially, with the writer already closed: commit key1, create an old-history branch, prepare key2 under generation0, truncate main, and verify that the exact prepared committable is rejected with generation changed. After table.fastForward("old-history"), retrying that same committable succeeds and a real Java read returns keys1/2. The branch legitimately restores key1, but must not revive the separately fenced key2 message. Reject guarded fast-forward too, or publish the branch contents with a fresh destination generation that cannot rewind; no concurrent writer is required for this failure.
There was a problem hiding this comment.
Fixed in 56d1206. Fast-forward now rejects promotion when either the source or destination branch enables capture or retains guard history, including after capture is disabled or the table is truncated. This covers the Java table API, direct filesystem branch manager, and Python. The regression closes the staged writer before truncation and verifies that rejected promotion leaves destination metadata unchanged and the same committable fenced.
| boolean resolveGuards = | ||
| !isStreaming || DeleteOrderingGuardFile.mayContainGuards(afterSnapshot); | ||
| Map<Pair<BinaryRow, Integer>, List<IndexFileMeta>> beforeGuards = | ||
| resolveGuards ? incrementalGuards(beforeSnapshot.get()) : Collections.emptyMap(); |
There was a problem hiding this comment.
[P1] Avoid requiring expired predecessor metadata for current APPEND deltas
This unconditionally resolves beforeSnapshot whenever the after-snapshot carries guard history, even when an APPEND delta has no before files. With a full-compacted delete guard, I appended a stale key1/version2 plus fresh key2/version5, expired all predecessors while retaining current snapshot3, and verified that its data/guard files and batch read remain valid. A public file-monitor stream restored at3 then fails here loading expired snapshot2; the identical unguarded control succeeds. Flink's default PK lookup FILE_MONITOR/AllDelta path also calls this code. Current-delta consumption should not require a deleted predecessor solely to discover an empty before side. Avoid that load when unnecessary, or preserve the required predecessor guard references independently of expired snapshot metadata.
There was a problem hiding this comment.
Fixed in 56d1206. Streaming delta planning now skips predecessor-snapshot loading when the before-file set is empty. Added a public file-monitor streaming regression that expires the predecessor snapshots, restores at the retained APPEND snapshot, and verifies that the stale row remains hidden while the fresh row is emitted. Both capture-enabled and capture-disabled histories are covered.
| rollbackProperties = | ||
| DeleteOrderingGuardPartitions.merge( | ||
| rollbackProperties, | ||
| targetSnapshot, |
There was a problem hiding this comment.
[P1] Validate restored guard policy before publishing rollback-as-latest
This path restores the target guard metadata/index manifest without checking it against the current persisted ordering policy. I followed the documented migration sequence: capture an ascending delete/version4, append hidden version2 and tag it, truncate the entire table, persist descending order, reopen and successfully write/read new data. rollbackToAsLatest of the old guarded target then returns true and advances the head, but a fresh public read of the latest table fails with Delete ordering guard ordering/deletion policy changed since schema 0; an ordering migration is required. Ordinary commits validate the segment headers; this bypass publishes an incompatible state first. Reject an incompatible target before publication, or atomically restore a compatible effective policy with its guards.
There was a problem hiding this comment.
Fixed in 56d1206. rollbackToAsLatest now validates the target guard segment headers against the current persisted ordering/deletion policy before publication, and revalidates if the schema changes during preparation. Added compatible and incompatible policy cases with capture enabled and disabled. Incompatible restoration leaves the current head unchanged and readable; compatible restoration with payload schema evolution succeeds.
Resolve persisted delete bounds at the merge wrapper boundary and capture one strongest bound per primary key. Remove redundant guard metadata and intermediate snapshot migration paths. Replace native guard rescaling and connector transport with early rejection in Flink and Spark. Update the tests and documentation.
Validate retained guard support when creating merge and raw readers to preserve deferred store initialization in PrimaryKeyFileStoreTable.newRead. Add a v15 manifest-committable fixture that includes guard generation, while retaining compatibility checks for the v13 and v14 fixtures. Validation: 232 targeted core tests pass with normal Maven checks.
Reject unsupported PyPaimon access to guarded histories and validate the observed guard set on every compaction publication attempt. Block destructive rollback that can revive fenced writers and validate reads from planned splits. Version manifest checkpoint envelopes, including Flink multi-table state, to reject unsupported downgrades clearly while retaining supported older-state reads. Document rollout restrictions and add regression coverage. Validation: 431 core tests, 5 Flink serializer tests, 205 Python tests, normal Maven checks, Python lint, and git diff --check.
Validate delete ordering guard support before catalog delegation or local rollback cleanup. Cover snapshot, tag, and timestamp rollback with capture enabled or retained guard history, preserving snapshot and tag metadata. Validation: 70 targeted Python tests; Python lint; git diff --check.
Resolve retained guard history from existing snapshots when a stale latest hint points to a missing file, preserving rollback recovery and writer fencing. Cover snapshot and tag rollback with capture enabled or disabled. Initialize Python planner, reader and writer test doubles with ordinary table metadata required by guard validation. Validation: 108 Java tests with normal Maven checks; 303 Python tests; changed-file Flake8; git diff --check.
Reject fast-forward when either branch enables delete ordering guards or retains guard history in Java and Python. Validate restored guard policies before rollback-to-as-latest publishes a snapshot. Keep streaming APPEND reads working when an empty before side no longer has a retained predecessor snapshot. Add regression coverage and document the fast-forward restriction. Validation: 90 targeted core tests with normal Maven checks; 72 Python tests; changed-file Flake8; git diff --check.
87347ad to
56d1206
Compare
Purpose
Add opt-in protection against late updates recreating rows after primary-key compaction removes their delete records. With ascending source versions, a captured delete at version
4continues to block an insert at version2, while an insert at version5is accepted.Enable
delete-ordering-guards.enabled=trueon fixed-bucket primary-key tables using thededuplicatemerge engine. The default isfalse, preserving existing behavior. Compaction captures the strongest delete bound for each key in an output guard segment, retaining keys and ordering metadata without deleted payloads. Disabling capture leaves committed guards active.sys.rescalewhile capture is enabled or committed guards remain active. An explicit overwrite or truncate resets ordering history in the replaced scope.Supported scope excludes append/Data Evolution tables, other merge engines, dynamic/postpone buckets, configured field aggregate functions, deletion vectors, primary-key indexes, chain tables, and Iceberg compatibility. Ignored deletes and record TTL expiry create no guards. Enabling capture cannot recover deletes already pruned.
Fixes #11019 for tables configured with fixed buckets and delete ordering guards enabled; the issue's original DDL requires these opt-in settings.
Tests
c5bd3d4, including core/integrations on JDK 8 and 11, Flink 1 and 2, Spark 3 and 4, and E2E suites.