hcrosse commented on code in PR #2468:
URL: https://github.com/apache/iceberg-go/pull/2468#discussion_r4516687564
##########
table/table.go:
##########
@@ -656,6 +659,9 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
for _, apply := range opts {
apply(&co)
}
+ if co.replay == nil {
+ co.replay = newStagedReplay(cmp.Or(co.branch, MainBranch),
updates, nil)
Review Comment:
`withCommitNoReplay` only makes an in-call conflict terminal, and that still
holds because `doCommit` returns on the first `ErrCommitFailed` before any
refresh. On a resume, main also ran the rebuild engine for these commits, with
the branch pinned and `forceRebuild` off, so running `stagedReplay` there is
the intended change and not a break.
##########
table/table.go:
##########
@@ -844,18 +850,7 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
// submit an older snapshot whose manifests it now
counts as
// superseded and deletes on success.
forceRebuild := co.resumes > 0 && attempt == 0 &&
!co.noReplay
- var (
- rebuiltUpdates []Update
- orphaned []string
- rebuildErr error
- step = "rebuild manifest list"
- )
- if co.replay != nil {
- step = "replay staged updates"
- rebuiltUpdates, orphaned, rebuildErr =
co.replay.replay(retryCtx, current, wfs, attemptBase+int(attempt), forceRebuild)
- } else {
- rebuiltUpdates, orphaned, rebuildErr =
rebuildSnapshotUpdates(retryCtx, updates, current, co.branch, wfs,
attemptBase+int(attempt), forceRebuild)
- }
+ rebuiltUpdates, orphaned, rebuildErr :=
co.replay.replay(retryCtx, current, wfs, attemptBase+int(attempt), forceRebuild)
Review Comment:
`withCommitNoReplay` only makes an in-call conflict terminal, and that still
holds because `doCommit` returns on the first `ErrCommitFailed` before any
refresh. On a resume, main also ran the rebuild engine for these commits, with
the branch pinned and `forceRebuild` off, so running `stagedReplay` there is
the intended change and not a break.
##########
table/table.go:
##########
@@ -932,12 +927,9 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
return nil, context.Cause(retryCtx)
}
- // Keep per-attempt expiry assertions out of reqs so refresh
does not
- // treat them as fixed staged requirements.
- submitReqs := reqs
- if co.replay != nil {
- submitReqs = append(slices.Clone(reqs),
expirationAssertions(current, co.replay.branch, co.replay.walkedRefs, reqs)...)
- }
+ // Keep replay assertions out of reqs so refresh does not treat
them as
+ // fixed staged requirements.
+ submitReqs := append(slices.Clone(reqs),
co.replay.requirements(current, reqs)...)
Review Comment:
`withCommitNoReplay` only makes an in-call conflict terminal, and that still
holds because `doCommit` returns on the first `ErrCommitFailed` before any
refresh. `requirements` adds assertions only for staged expiries, which main
already added on a resume of these commits.
--
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]
