wgtmac commented on code in PR #890:
URL: https://github.com/apache/iceberg-cpp/pull/890#discussion_r3838972507
##########
src/iceberg/table_metadata.cc:
##########
@@ -1106,6 +1107,8 @@ Status
TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot> snapsho
metadata_.next_row_id += add_rows.value();
}
+ ICEBERG_LOG_DEBUG("Added snapshot {} (sequence number {}) to table metadata",
Review Comment:
Could we drop this DEBUG log to stay closer to Java? `AddSnapshot` is a
low-level builder operation and may run for temporary or retry metadata that is
never committed; Java logs only after the snapshot commit succeeds.
##########
src/iceberg/transaction.cc:
##########
@@ -375,15 +377,55 @@ Result<std::shared_ptr<Table>> Transaction::Commit() {
int32_t max_wait_ms = props.Get(TableProperties::kCommitMaxRetryWaitMs);
int32_t total_timeout_ms =
props.Get(TableProperties::kCommitTotalRetryTimeMs);
+ // Snapshot id before the commit, to detect whether this commit advanced it
(a
+ // data commit) versus a metadata-only commit that adds no snapshot.
+ const int64_t base_current_snapshot_id =
ctx_->table->metadata()->current_snapshot_id;
bool is_first_attempt = true;
+ int32_t attempt = 0;
+ std::string last_error;
auto commit_result =
MakeCommitRetryRunner(num_retries, min_wait_ms, max_wait_ms,
total_timeout_ms)
- .Run([this, &is_first_attempt]() -> Result<std::shared_ptr<Table>> {
+ .Run([this, &is_first_attempt, &attempt,
+ &last_error]() -> Result<std::shared_ptr<Table>> {
+ ++attempt;
+ // The runner only re-invokes this task when it has decided to
retry, so
+ // attempt > 1 here means a genuine retry after a retryable
failure.
+ if (attempt > 1) {
+ ICEBERG_LOG_WARN("Retrying transaction commit (attempt {})
after: {}",
+ attempt, last_error);
+ }
auto result = CommitOnce(is_first_attempt);
is_first_attempt = false;
+ if (!result.has_value()) {
+ last_error = result.error().message;
+ }
return result;
});
+ if (commit_result.has_value()) {
+ // Name the resulting snapshot only when this commit produced one (current
+ // snapshot advanced); metadata-only commits report a plain success.
+ std::string detail;
+ if (auto snapshot = commit_result.value()->metadata()->Snapshot();
+ snapshot.has_value() &&
+ snapshot.value()->snapshot_id != base_current_snapshot_id) {
+ const auto& summary = snapshot.value()->summary;
+ auto op = summary.find(SnapshotSummaryFields::kOperation);
+ detail =
+ std::format(": committed snapshot {} (op={})",
snapshot.value()->snapshot_id,
+ op != summary.end() ? op->second : "unknown");
+ }
+ if (attempt > 1) {
+ ICEBERG_LOG_INFO("Transaction commit succeeded after {} attempts{}",
attempt,
+ detail);
+ } else {
+ ICEBERG_LOG_INFO("Transaction commit succeeded{}", detail);
+ }
+ } else {
+ ICEBERG_LOG_ERROR("Transaction commit failed after {} attempt(s): {}",
attempt,
Review Comment:
Java propagates the final commit error without a generic ERROR log. Also,
`CommitStateUnknown` is not a confirmed failure. Could we remove this log or
distinguish the unknown commit state explicitly?
--
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]
