wgtmac commented on code in PR #576:
URL: https://github.com/apache/iceberg-cpp/pull/576#discussion_r2930169869
##########
src/iceberg/transaction.cc:
##########
@@ -343,6 +347,49 @@ Result<std::shared_ptr<Table>> Transaction::Commit() {
return table_;
}
+Result<std::shared_ptr<Table>> Transaction::CommitOnce() {
+ auto refresh_result = table_->Refresh();
+ if (!refresh_result.has_value()) {
+ return std::unexpected(refresh_result.error());
+ }
+
+ if (metadata_builder_->base() != table_->metadata().get()) {
Review Comment:
`CommitOnce()` relies on `weak_update.lock()` to re-apply updates over
refreshed metadata during a retry. Because `pending_updates_` holds
`std::weak_ptr`, if a caller does not explicitly retain the
`shared_ptr<PendingUpdate>` instance returned by `tx->New*()` methods (e.g.,
standard chained usage like
`tx->NewUpdateSchema().value()->AddColumn(...)->Commit();`), the pointer
expires. On retry, the expired update is silently skipped, resulting in a
partially committed transaction that drops the user's modifications. A
transaction must strongly own its updates to guarantee safe retries. Consider
changing `pending_updates_` to use `std::shared_ptr<PendingUpdate>` and resolve
the circular dependency by having `PendingUpdate` hold a
`std::weak_ptr<Transaction>`.
--
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]
