laskoviymishka commented on code in PR #1605:
URL: https://github.com/apache/iceberg-go/pull/1605#discussion_r3681120091
##########
catalog/hive/lock_test.go:
##########
@@ -323,6 +323,35 @@ func TestReleaseLock(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestReleaseLockForCleanupUsesLiveBoundedContext(t *testing.T) {
Review Comment:
These two tests only cover `releaseForCleanup` in isolation — neither goes
through the DropTable/RenameTable/CommitTable defers, so nothing here catches
the success-path regression above. A test that drives CommitTable with Unlock
failing after a successful AlterTable, then asserts we get the committed
metadata back rather than a nil-metadata error, would fail today and lock in
the fix.
##########
catalog/hive/hive.go:
##########
@@ -428,7 +428,7 @@ func (c *Catalog) DropTable(ctx context.Context, identifier
table.Identifier) er
return fmt.Errorf("%w: failed to acquire lock for %s.%s: %w",
table.ErrCommitFailed, database, tableName, err)
}
defer func() {
- _ = lock.Release(ctx)
+ err = errors.Join(err, lock.releaseForCleanup(ctx))
Review Comment:
Same success-path issue as the CommitTable defer below — if the drop lands
but `releaseForCleanup` fails, this overwrites the nil `err`, so the caller
sees a failure for a table that's already gone and a retry hits
`ErrNoSuchTable`. I'd only join the release error when `err` is already
non-nil, and log-and-drop it otherwise.
##########
catalog/hive/hive.go:
##########
@@ -502,7 +502,7 @@ func (c *Catalog) RenameTable(ctx context.Context, from, to
table.Identifier) (*
table.ErrCommitFailed, fromDB, fromTable, toDB,
toTable, err)
}
defer func() {
- _ = lock.Release(ctx)
+ err = errors.Join(err, lock.releaseForCleanup(ctx))
Review Comment:
Same issue, slightly worse here: the success path ends `return
c.LoadTable(ctx, to)`, so a landed rename with a failed unlock hands the caller
a valid `*table.Table` and a non-nil error together, which breaks the usual
(value, error) contract. Same conditional-join guard fixes it.
##########
catalog/hive/hive.go:
##########
@@ -548,7 +548,7 @@ func (c *Catalog) CommitTable(ctx context.Context,
identifier table.Identifier,
table.ErrCommitFailed, database, tableName, err)
}
defer func() {
- _ = lock.Release(ctx)
+ err = errors.Join(err, lock.releaseForCleanup(ctx))
Review Comment:
This is the one I'd hold the PR on. Joining the release error into `err`
unconditionally makes the CommitTable success path worse than the bug it fixes.
When the commit lands we `return staged.Metadata(),
staged.MetadataLocation(), nil`, then this defer overwrites `err` with the
release error. doCommit in `table/table.go` then sees a non-nil err that
doesn't wrap `ErrCommitFailed`, takes the unknown-state branch (`cleanupOrphans
= false; return nil, err`), and hands the caller `(nil, unlockErr)` for a
commit that's durably in the metastore — so it retries or aborts a write that
already succeeded. Wrapping in `ErrCommitFailed` isn't the fix either; that'd
retry a landed commit.
I'd surface it out-of-band — only join when the op already failed, otherwise
log:
```go
defer func() {
if releaseErr := lock.releaseForCleanup(ctx); releaseErr != nil {
if err != nil {
err = errors.Join(err, releaseErr)
} else {
log.Printf("hive: lock release failed after a successful commit:
%v", releaseErr)
}
}
}()
```
Same guard on the DropTable and RenameTable defers.
--
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]
