laskoviymishka commented on code in PR #1603:
URL: https://github.com/apache/iceberg-go/pull/1603#discussion_r3677875046
##########
table/transaction.go:
##########
@@ -2384,6 +2382,8 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
return tbl, err
}
+ t.committed = true
Review Comment:
Setting `committed` here — after the catalog accepts but before `PostCommit`
— is the right call: a `PostCommit` failure must not leave the transaction
retriable, or we'd re-run the catalog commit. Worth a one-line comment saying
exactly that, since the obvious "improvement" is to move it below the loop,
which would quietly reintroduce the bug.
##########
table/transaction.go:
##########
@@ -2384,6 +2382,8 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
return tbl, err
Review Comment:
One thing this change surfaces: `doCommit` treats a clean 409
(`ErrCommitFailed`) differently from a 5xx/timeout, where the commit state is
genuinely unknown — the catalog may have accepted it silently. Now that any
failure here leaves `committed == false`, a caller who retries after an
unknown-state error could double-apply the snapshot if the first attempt
actually landed.
Java guards this by never retrying `CommitStateUnknownException`. I'd
distinguish the failure class before allowing the retry — e.g. treat a
non-`ErrCommitFailed` failure as terminal (still surface the error, but don't
leave the transaction retriable). Happy to punt it to a follow-up if you'd
rather keep this PR narrow, but I'd want it tracked. wdyt?
##########
table/commit_retry_test.go:
##########
@@ -944,3 +944,32 @@ func TestDoCommit_RetryProgressesFreshMeta(t *testing.T) {
require.NotContains(t, wfs.files, cat.observedManifestLists[1],
"attempt-1 rebuild manifest list must be cleaned as orphan
after success")
}
+
+func TestTransactionCommit_MarksCommittedOnlyOnSuccess(t *testing.T) {
+ // first CommitTable : fails (non-ErrCommitFailed, so doCommit returns
immediately), the second succeeds
+ cat := &sequentialCatalog{
+ errs: []error{errors.New("catalog unavailable")},
+ }
+ tbl := newRetryTestTable(t, cat, nil)
+ cat.metadata = tbl.Metadata()
+
+ tx := tbl.NewTransaction()
+ require.NoError(t, tx.SetProperties(map[string]string{"key": "value"}))
+
+ _, err := tx.Commit(t.Context())
+ require.Error(t, err, "first commit must surface the catalog failure")
+ assert.False(t, tx.committed, "failed commit must leave committed ==
false")
+
+ // transaction stays usable: further changes and a retry are both
allowed
+ require.NoError(t, tx.SetProperties(map[string]string{"key2":
"value2"}),
+ "apply must be allowed after a failed commit")
+
+ committed, err := tx.Commit(t.Context())
Review Comment:
Could we assert `cat.attempts` went 1→2 here? Right now nothing proves the
second `Commit` actually reached the catalog rather than short-circuiting — the
other tests in this file pin that with `cat.attempts.Load()`.
While we're here, a sub-case where `doCommit` exhausts its internal retries
on `ErrCommitFailed` (and still leaves `committed == false`) would cover the
other failure path this fix touches.
##########
table/transaction.go:
##########
@@ -2372,8 +2372,6 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
return nil, errors.New("transaction has already been committed")
}
- t.committed = true
-
if len(meta.updates) > 0 {
t.reqs = append(t.reqs, AssertTableUUID(meta.uuid))
Review Comment:
This append runs on every `Commit()` call, and now that we intentionally
allow a retry after a failed `doCommit`, a second `Commit()` tacks on another
`AssertTableUUID` for the same UUID. `apply()` dedupes requirements via
`requirementSemanticKey`, but this append bypasses `apply()` entirely, so
nothing collapses them — the `reqs` slice grows by one on every retry.
Lenient catalogs shrug it off, but a strict REST catalog will reject the
duplicate requirement, which defeats the retry this PR is trying to enable. I'd
guard the append so it fires at most once (check for an existing
`assertTableUUID`, or save `len(t.reqs)` before the append and truncate back on
failure).
The new test won't catch this today — `sequentialCatalog.CommitTable`
ignores `reqs` — so asserting the `reqs` contents after the retry would pin it.
wdyt?
##########
table/commit_retry_test.go:
##########
@@ -944,3 +944,32 @@ func TestDoCommit_RetryProgressesFreshMeta(t *testing.T) {
require.NotContains(t, wfs.files, cat.observedManifestLists[1],
"attempt-1 rebuild manifest list must be cleaned as orphan
after success")
}
+
+func TestTransactionCommit_MarksCommittedOnlyOnSuccess(t *testing.T) {
+ // first CommitTable : fails (non-ErrCommitFailed, so doCommit returns
immediately), the second succeeds
+ cat := &sequentialCatalog{
+ errs: []error{errors.New("catalog unavailable")},
+ }
+ tbl := newRetryTestTable(t, cat, nil)
+ cat.metadata = tbl.Metadata()
+
+ tx := tbl.NewTransaction()
+ require.NoError(t, tx.SetProperties(map[string]string{"key": "value"}))
+
+ _, err := tx.Commit(t.Context())
+ require.Error(t, err, "first commit must surface the catalog failure")
+ assert.False(t, tx.committed, "failed commit must leave committed ==
false")
+
+ // transaction stays usable: further changes and a retry are both
allowed
+ require.NoError(t, tx.SetProperties(map[string]string{"key2":
"value2"}),
+ "apply must be allowed after a failed commit")
+
+ committed, err := tx.Commit(t.Context())
+ require.NoError(t, err, "commit retry must be allowed after a transient
failure")
+ require.NotNil(t, committed)
+ assert.True(t, tx.committed, "committed must be set only after a
successful commit")
+
+ _, err = tx.Commit(t.Context())
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "already been committed")
Review Comment:
`assert.ErrorContains(t, err, "already been committed")` reads cleaner here
and matches the rest of the file — it nil-checks internally, so we skip
reaching through `err.Error()`.
--
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]
