-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[iceberg] Do not fail the commit when the Iceberg REST catalog rejects or is unsure - #9749
[iceberg] Do not fail the commit when the Iceberg REST catalog rejects or is unsure #9749arthurgaubil wants to merge 1 commit 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
JingsongLi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Uh oh!
There was an error while loading. Please reload this page.
Purpose
Closes #8875.
IcebergRestMetadataCommitter.commitMetadataImplcallsTableOperations.commit()directly and lets every exception escape. Because the Iceberg sync runs inside a Paimon commit callback, which under Flink runs insidenotifyCheckpointComplete, any exception there is fatal and restarts the whole job. For a job syncing many tables through the multiplexed sink, one table losing a commit race takes down all of them.Two exceptions from that call don't warrant it:
CommitStateUnknownException— the catalog returned an ambiguous response (e.g. a 500 or timeout) and we cannot tell whether the commit applied. Failing does not resolve the ambiguity.CommitFailedException— the compare-and-swap was rejected because the table moved between our read of the base metadata and the commit. This one is unambiguous: it implementsCleanableFailure, so nothing landed server-side.In both cases the committer already reconciles on its own. The next attempt reloads the table from the catalog and runs
checkBase()against that live state: if the commit landed, the base matches and the next commit proceeds normally; if it did not,checkBase()sees the drift and the table is rebuilt from the current file set. Paimon's own commit has already durably applied the data, so the only consequence of not failing is that the Iceberg metadata lags by one commit.This logs a warning for those two and lets the next attempt reconcile. Any other exception still propagates unchanged, so genuine failures (auth, misconfiguration, unreachable catalog) are unaffected.
One alternative considered was retrying in place rather than deferring. That is a larger change than it looks:
updatedForCommitis built from a base captured earlier in the method, so re-callingcommit()fails identically, a real retry has to redocommitMetadataImplfrom the top. Deferring to the next commit gets the same reconciliation for free, since that path already exists and already runs every checkpoint. Happy to go the retry route instead if maintainers prefer it.We could also gate this behind a feature flag if prefered
Tests
No new tests. The change adds two
catchclauses that log and return on the existing reconcile path; existing coverage ofcommitMetadataImplis unchanged, and any other exception still propagates as before. Glad to add a test if you'd like one — reproducing it needs the REST catalog'scommit()stubbed to throw, so it would come with a small mock harness.