[iceberg] Retry REST metadata publication before cleanup - #9749
arthurgaubil wants to merge 2 commits into
Conversation
…s or is unsure IcebergRestMetadataCommitter calls TableOperations.commit() directly and lets every exception escape. Because the Iceberg sync runs inside a Paimon commit callback, which for Flink runs inside notifyCheckpointComplete, any exception there is fatal: the whole job restarts. For a job syncing many tables, one table losing a commit race stops all of them. Two of these exceptions do not warrant that. CommitStateUnknownException means the outcome is unknown, and CommitFailedException means the compare-and-swap was rejected and nothing was applied (it implements CleanableFailure). In both cases the next commit attempt reloads the table and runs checkBase() against the live catalog state, which either matches and proceeds or detects the drift and rebuilds from the current file set. Paimon's own commit has already durably applied the data, so only the Iceberg metadata lags, by one commit. Log a warning and let the next attempt reconcile instead of failing the job. Any other exception still propagates unchanged. Closes apache#8875
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed 00dc939. Requirement fit: the multi-table job-restart problem in #8875 is real, but this solution needs a change in the publication/retry contract. There is one P1 finding below.
The next Paimon commit is not a guaranteed recovery mechanism: an idle or bounded writer may never make another commit, and a successful return also releases callback cleanup immediately. Please add failure-injection coverage for rejected publication, ambiguous responses with both applied/not-applied outcomes, retention cleanup, and the final commit. This review is based on the exact committer/callback/Flink state transitions; no new REST failure harness was run.
| ? null | ||
| : updatedForCommit.currentSnapshot().snapshotId(), | ||
| e); | ||
| } catch (CommitFailedException e) { |
There was a problem hiding this comment.
[P1] Do not report publication success before the REST state is reconciled
Returning normally from these catches tells IcebergCommitCallback that the external catalog serves the new head. It then runs deleteApplicableMetadataFiles and expireManifestList immediately (callback lines 1266–1274; the no-base path also calls expireAllBefore). For example, with Iceberg snapshot retention min=max=2, REST snapshot 2 still advertises snapshots 1 and 2. If publication of snapshot 3 is rejected here, cleanup can delete snapshot 1's manifest list even though the REST metadata never removed that snapshot, breaking catalog time-travel reads. On a final/idle commit there is also no guaranteed next attempt: Flink clears the successful committable and the callback retains no durable retry obligation. Preserve failed/unknown publication state so cleanup cannot remove externally referenced files, and either reconcile/retry before success or retain a durable retry task with a verified retention boundary. Add a rejected-publication test that asserts every file referenced by the unchanged REST metadata remains readable.
There was a problem hiding this comment.
You’re right. I changed the committer to retry from fresh catalog state and only return after publication is confirmed. Exhausted retries now propagate, so cleanup is skipped and the durable retry obligation is preserved. I also added failure-injection coverage for rejected publication, ambiguous applied/not-applied outcomes, retention safety, and the final commit.
Addressed in 969f70f
Also let me know if you would want MAX_COMMIT_ATTEMPTS to be exposed and customizable though IcebergOptions
arthurgaubil
changed the title
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed the REST metadata-publication retry path. Retrying typed ambiguous commit failures while reloading the REST catalog state lets retries reconcile a commit that may already have succeeded instead of reporting a false success or repeatedly rebuilding state. The direction and tests look good to me.
2 participants
Purpose
Closes #8875.
Retry rejected and ambiguous REST metadata commits from freshly loaded catalog state before reporting publication success. If an ambiguous commit already landed, the existing Paimon commit-identity check recognizes it. If publication still cannot be confirmed after three attempts, the error propagates so callback cleanup cannot remove files referenced by the previous REST head and Flink retains its durable retry behavior.
Tests
mvn -o -Ppaimon-iceberg -pl paimon-iceberg -DwildcardSuites=none -Dtest=IcebergRestMetadataCommitterTest testAll 27 tests passed. The new failure-injection coverage exercises rejected commits, ambiguous applied and not-applied outcomes, retry exhaustion on a final commit, and verifies that metadata and manifests referenced by the unchanged REST head remain readable.