badalprasadsingh commented on code in PR #1858:
URL: https://github.com/apache/iceberg-go/pull/1858#discussion_r3853431295
##########
catalog/multi_table_transaction.go:
##########
@@ -102,8 +102,10 @@ func (m *MultiTableTransaction) AddTransaction(tx
*table.Transaction) error {
// Commit extracts pending changes from all added transactions and
// commits them atomically. On success, all transactions are marked
-// as committed. On failure, no transactions are marked committed
-// and the caller may retry.
+// as committed. On failure, no transactions are marked committed.
+//
+// A retry must be rebuilt from freshly loaded tables:
Review Comment:
Done.
I filtered this in `Commit`. Documenting at `AddTransaction` wouldn't work -
a txn can gain updates after being added. Tests added.
##########
catalog/multi_table_transaction_test.go:
##########
@@ -277,3 +277,78 @@ func TestCommitAndReloadPartialFailure(t *testing.T) {
// First table was loaded successfully before the second failed.
assert.Len(t, tables, 1)
}
+
+func mtxTableWithHead(t *testing.T, name string, headID, childID int64)
(*table.Table, table.Metadata) {
+ t.Helper()
+
+ base := mtxTestTable(t, "db", name).Metadata()
+ withHead := mtxGraftSnapshot(t, base, headID, nil)
+ advanced := mtxGraftSnapshot(t, withHead, childID, &headID)
+
+ return table.New(table.Identifier{"db", name}, withHead, "", nil, nil),
advanced
+}
+
+func mtxGraftSnapshot(t *testing.T, base table.Metadata, id int64, parent
*int64) table.Metadata {
+ t.Helper()
+
+ builder, err := table.MetadataBuilderFromBase(base, "")
+ require.NoError(t, err)
+ require.NoError(t, builder.AddSnapshot(&table.Snapshot{
+ SnapshotID: id,
+ ParentSnapshotID: parent,
+ SequenceNumber: base.LastSequenceNumber() + 1,
+ TimestampMs: base.LastUpdatedMillis() + 1,
+ Summary: &table.Summary{Operation: table.OpAppend},
+ }))
+ require.NoError(t, builder.SetSnapshotRef(table.MainBranch, id,
table.BranchRef))
+ out, err := builder.Build()
+ require.NoError(t, err)
+
+ return out
+}
+
+// Why: with a distinct head per table, a shared or missing assertion would
still let one table's concurrent writer through.
+func TestMultiTableTransactionFencesEachBranchHead(t *testing.T) {
+ tbl1, advanced1 := mtxTableWithHead(t, "t1", 100, 101)
+ tbl2, advanced2 := mtxTableWithHead(t, "t2", 200, 201)
+
+ stub := &stubCatalog{}
+ mtx := &MultiTableTransaction{cat: stub}
+
+ for _, tbl := range []*table.Table{tbl1, tbl2} {
+ tx := tbl.NewTransaction()
+ require.NoError(t,
tx.SetProperties(map[string]string{"offsets": "42"}))
+ require.NoError(t, mtx.AddTransaction(tx))
+ }
+
+ require.NoError(t, mtx.Commit(context.Background()))
Review Comment:
Done.
--
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]
