wgtmac commented on code in PR #774:
URL: https://github.com/apache/iceberg-cpp/pull/774#discussion_r3670761910
##########
src/iceberg/update/pending_update.cc:
##########
@@ -35,6 +35,10 @@ Status PendingUpdate::Commit() {
if (!ctx_->transaction) {
// Table-created path: no transaction exists yet, create a temporary one.
ICEBERG_ASSIGN_OR_RAISE(auto txn, Transaction::Make(ctx_));
+ auto self = weak_from_this().lock();
+ if (self) {
+ ICEBERG_RETURN_UNEXPECTED(txn->AddUpdate(self));
+ }
Review Comment:
Please require shared ownership and always register the update.
```suggestion
auto self = weak_from_this().lock();
ICEBERG_PRECHECK(self != nullptr,
"PendingUpdate must be owned by std::shared_ptr");
ICEBERG_RETURN_UNEXPECTED(txn->AddUpdate(self));
```
##########
src/iceberg/update/pending_update.cc:
##########
@@ -35,6 +35,10 @@ Status PendingUpdate::Commit() {
if (!ctx_->transaction) {
// Table-created path: no transaction exists yet, create a temporary one.
ICEBERG_ASSIGN_OR_RAISE(auto txn, Transaction::Make(ctx_));
Review Comment:
Please keep this temporary transaction detached. Remove `ctx->transaction =
...` from `Transaction::Make(ctx)`; only explicit transactions should set it.
Otherwise the next `Commit()` sees an expired transaction.
##########
src/iceberg/update/pending_update.cc:
##########
@@ -43,11 +47,15 @@ Status PendingUpdate::Commit() {
auto commit_result = txn->Commit();
if (!commit_result.has_value()) {
- std::ignore = Finalize(std::unexpected(commit_result.error()));
+ if (!self) {
+ std::ignore = Finalize(std::unexpected(commit_result.error()));
+ }
return std::unexpected(commit_result.error());
}
- std::ignore = Finalize(commit_result.value()->metadata().get());
+ if (!self) {
Review Comment:
Please route the empty-update branch in `Transaction::Commit()` through the
normal finalization block. Otherwise registered no-op updates skip `Finalize()`.
--
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]
