JingsongLi commented on code in PR #9749:
URL: https://github.com/apache/paimon/pull/9749#discussion_r4180279233
##########
paimon-iceberg/src/main/java/org/apache/paimon/iceberg/IcebergRestMetadataCommitter.java:
##########
@@ -154,13 +158,26 @@ public void commitMetadata(Path newMetadataPath,
@Nullable Path baseMetadataPath
@Override
public void commitMetadata(
IcebergMetadata newIcebergMetadata, @Nullable IcebergMetadata
baseIcebergMetadata) {
+ int commitAttempt = 1;
+ int unknownHostRetry = 0;
long delayMillis = unknownHostInitialRetryDelayMillis;
- for (int retry = 0; ; retry++) {
+ while (true) {
try {
commitMetadataImpl(newIcebergMetadata, baseIcebergMetadata);
return;
+ } catch (CommitStateUnknownException | CommitFailedException e) {
+ if (commitAttempt == MAX_COMMIT_ATTEMPTS) {
+ throw commitFailure(e);
+ }
+ LOG.warn(
+ "Commit attempt {} to rest catalog failed for table
{}; reloading catalog"
+ + " state before retrying.",
+ commitAttempt,
+ icebergTableIdentifier,
+ e);
+ commitAttempt++;
Review Comment:
[P1] Revalidate publication eligibility before retrying old metadata
The target metadata stays fixed across these retries, while another writer
can publish a newer Paimon/REST snapshot. I reproduced this through two public
commit users: while writer A is publishing snapshot 2, writer B commits and
normally publishes snapshot 3; A's original REST CAS is correctly rejected. The
added retry reloads REST head 3, treats it as an incorrect base, and
drops/recreates the REST table at snapshot 2. A then returns success although
Iceberg reads only 2 of the 3 committed rows. The same real conflict without
the new retry preserves head 3 and propagates the error. Callbacks execute
after the snapshot commit lock, so this interleaving is allowed. A bounded/idle
writer may never make another commit to repair this rollback. Before
retry/reconciliation, recheck that the target Paimon snapshot and commit
identity are still eligible for publication; if superseded, safely coordinate
or retain the failure instead of destroying the newer REST head. Please add
this concu
rrent-publication regression test. Comparing only numeric snapshot IDs would
also need to account for legitimate rollback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
