sshivampeta commented on code in PR #16289:
URL: https://github.com/apache/iceberg/pull/16289#discussion_r3278348846
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -320,10 +320,13 @@ private void commitReplaceTransaction(boolean orCreate) {
}
}
- // because this is a replace table, it will always completely
replace the table
- // metadata. even if it was just updated.
- if (base != underlyingOps.current()) {
- this.base = underlyingOps.current(); // just refreshed
+ // Replace transactions must not silently overwrite concurrent
commits. If the table
+ // metadata has changed since the transaction started, fail
instead of rebasing and
+ // merging staged updates.
+ if (base != null && underlyingOps.current() != base) {
Review Comment:
Good catch. Since onlyRetryOn(CommitFailedException.class) retries on
CommitFailedException, throwing it directly inside the lambda would cause the
retry loop to keep hitting the same concurrent-modification check (since base
is never updated) until retries are exhausted — wasteful.
Fixed: now wrapping the CommitFailedException in
PendingUpdateFailedException (the same pattern used by applyUpdates in
commitSimpleTransaction) to immediately break the retry loop. Added a matching
catch (PendingUpdateFailedException e) block in commitReplaceTransaction that
cleans up and re-throws the unwrapped CommitFailedException
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
