linguoxuan commented on code in PR #576:
URL: https://github.com/apache/iceberg-cpp/pull/576#discussion_r2986921930
##########
src/iceberg/update/update_snapshot_reference.h:
##########
@@ -134,6 +134,8 @@ class ICEBERG_EXPORT UpdateSnapshotReference : public
PendingUpdate {
Kind kind() const final { return Kind::kUpdateSnapshotReference; }
+ bool IsRetryable() const override { return false; }
Review Comment:
When used within an external transaction, SnapshotManager.commit() does not
call transaction.commitTransaction():
```
// SnapshotManager.java
public void commit() {
commitIfRefUpdatesExist();
if (!isExternalTransaction) {
transaction.commitTransaction();
}
}
```
The retry then happens in the outer
BaseTransaction.commitSimpleTransaction() → applyUpdates(). During retry,
applyUpdates() resets this.current = underlyingOps.current() (refreshed
metadata), then re-calls update.commit(). But UpdateSnapshotReferencesOperation
holds this.base captured at construction time (the old metadata). When commit()
calls ops.commit(base, updated), TransactionTableOperations.commit() checks:
```
// BaseTransaction.java
public void commit(TableMetadata underlyingBase, TableMetadata metadata) {
if (underlyingBase != current) {
throw new CommitFailedException("Table metadata refresh is
required");
}
...
}
```
CommitFailedException is caught by applyUpdates() and wrapped as
PendingUpdateFailedException (which is not CommitFailedException), terminating
the retry loop immediately.
```
private void applyUpdates(TableOperations underlyingOps) {
if (base != underlyingOps.refresh()) {
// use refreshed the metadata
this.base = underlyingOps.current();
this.current = underlyingOps.current();
for (PendingUpdate update : updates) {
// re-commit each update in the chain to apply it and update current
try {
update.commit();
} catch (CommitFailedException e) {
// Cannot pass even with retry due to conflicting metadata
changes. So, break the
// retry-loop.
throw new PendingUpdateFailedException(e);
}
}
}
}
```
So Java's UpdateSnapshotReferencesOperation also cannot be retried within an
external transaction. Our IsRetryable() = false is consistent with Java's
behavior.
@wgtmac The architectures of C++ and Java differ somewhat; consequently, the
modifications required when `IsRetryable` returns `true` could be quite
substantial.
--
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]
