mzzz-zzm commented on code in PR #982:
URL: https://github.com/apache/iceberg-go/pull/982#discussion_r3202139760
##########
table/snapshot_producers.go:
##########
@@ -903,9 +943,103 @@ func (sp *snapshotProducer) commit(ctx context.Context)
(_ []Update, _ []Require
})
}
+ // Build the manifest-list rebuild closure. It is called by doCommit
+ // on each OCC retry to regenerate the manifest list so it correctly
+ // inherits all data files committed by concurrent writers since the
+ // original snapshot was built.
+ formatVersion := sp.txn.meta.formatVersion
+ snapshotID := sp.snapshotID
+ commitUUID := sp.commitUuid
+ capturedSnapshot := snapshot // copy the value so the closure is
self-contained
+ processManifestsFn := func(m []iceberg.ManifestFile)
([]iceberg.ManifestFile, error) {
+ return sp.processManifests(m)
+ }
+
+ rebuildFn := func(_ context.Context, freshParent *Snapshot, fio
iceio.WriteFileIO, attempt int) (_ *Snapshot, retErr error) {
+ // Load inherited manifests from the fresh parent.
+ var inherited []iceberg.ManifestFile
+ if freshParent != nil {
+ inherited, retErr = freshParent.Manifests(fio)
+ if retErr != nil {
+ return nil, fmt.Errorf("rebuild manifest list:
load parent manifests: %w", retErr)
+ }
+ }
+
+ // Combine own manifests with inherited ones, applying any
+ // producer-specific processing (no-op for fast/merge-append).
+ combined, procErr :=
processManifestsFn(slices.Concat(ownManifests, inherited))
+ if procErr != nil {
+ return nil, fmt.Errorf("rebuild manifest list: process
manifests: %w", procErr)
+ }
+
+ // Derive the sequence number. When there is a fresh parent,
use its
+ // sequence number + 1 so the rebuilt snapshot is strictly
greater than
+ // any committed peer. When there is no fresh parent (first
snapshot in
+ // the table or unknown parent), preserve the original sequence
number
+ // from the initial build.
+ var newSeq int64
+ if freshParent != nil && formatVersion >= 2 {
+ newSeq = freshParent.SequenceNumber + 1
+ } else {
+ newSeq = capturedSnapshot.SequenceNumber
+ }
+
+ // Write the rebuilt manifest list to a path unique to this
retry
+ // attempt. Each retry uses a different attempt counter in the
filename
+ // (snap-{id}-{attempt}-{uuid}.avro) so that S3
conditional-write
+ // semantics (if-none-match) do not reject the overwrite.
Orphaned files
+ // from superseded retry attempts are removed by doCommit after
the
+ // commit succeeds.
+ fname := newManifestListFileName(snapshotID, attempt,
commitUUID)
+ manifestListPath := locProvider.NewMetadataLocation(fname)
+
+ out, createErr := fio.Create(manifestListPath)
+ if createErr != nil {
+ return nil, fmt.Errorf("rebuild manifest list: create
file: %w", createErr)
+ }
+ defer internal.CheckedClose(out, &retErr)
+
+ var parentID *int64
+ if freshParent != nil {
+ id := freshParent.SnapshotID
+ parentID = &id
+ }
+
+ firstRowID := int64(0)
Review Comment:
Inside `rebuildFn` (`table/snapshot_producers.go`), `firstRowID` is now
derived from `freshMeta.NextRowID()` for v3 tables (only `freshMeta` is the
source of truth — `capturedSnapshot` is never consulted for this value):
```go
var firstRowID int64
if formatVersion >= 3 {
// Derive firstRowID from the fresh metadata so the manifest-list
// first-row-id header matches the catalog's current nextRowID.
firstRowID = freshMeta.NextRowID()
...
}
```
After `writer.AddManifests(combined)`, `rebuilt.FirstRowID` and
`rebuilt.AddedRows` are updated to reflect the rows actually written this
attempt:
```go
rebuilt.FirstRowID = &firstRowID
rebuilt.AddedRows = &addedRows // writer.NextRowID() - firstRowID
```
This mirrors what `commit()` does at the initial write site, keeping the
snapshot record and the manifest-list header consistent.
Tests added: `TestRebuildFn_V3FirstRowIDDerivedFromFreshMeta` (concurrent
write advances `NextRowID` to 50; asserts rebuilt `FirstRowID == 50` AND
`AddedRows == 1` reflecting only this attempt's contribution),
`TestRebuildFn_V3FirstRowIDZeroWhenNilNextRowID` (freshMeta with `NextRowID()
== 0` — the zero-equivalent for the `int64` API — asserts `FirstRowID == 0`,
`AddedRows == 1`, no panic).
--
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]
