laskoviymishka commented on code in PR #1610:
URL: https://github.com/apache/iceberg-go/pull/1610#discussion_r3711440588
##########
manifest.go:
##########
@@ -1358,16 +1359,18 @@ func NewManifestWriter(version int, out io.Writer, spec
PartitionSpec, schema *S
func (w *ManifestWriter) Close() error {
if w.closed {
- return nil
+ return w.closeErr
}
+ w.closed = true
+ var emptyErr error
if w.addedFiles+w.existingFiles+w.deletedFiles == 0 {
- return errors.New("empty manifest file has been written")
+ emptyErr = errors.New("empty manifest file has been written")
}
- w.closed = true
+ w.closeErr = errors.Join(emptyErr, w.writer.Close())
Review Comment:
This line does two things I'd tighten. `w.writer.Close()` runs
unconditionally now, so an empty close where the writer was never built
(`NewManifestWriter` can hand back a non-nil `w` alongside a non-nil error)
panics where the old early return returned cleanly. And `errors.Join` wraps the
empty-manifest error in a `*joinError`, so `errors.As` and sentinel/`==` checks
against it stop matching (`errors.Is` still works).
Both go away if we guard the writer and only join when there's a real second
error:
```go
var writerErr error
if w.writer != nil {
writerErr = w.writer.Close()
}
if emptyErr != nil {
w.closeErr = errors.Join(emptyErr, writerErr)
} else {
w.closeErr = writerErr
}
```
Exposure is narrow today (callers check the constructor error, no exported
sentinel yet), but it's a cheap fix that keeps the contract clean. wdyt?
##########
manifest.go:
##########
@@ -1358,16 +1359,18 @@ func NewManifestWriter(version int, out io.Writer, spec
PartitionSpec, schema *S
func (w *ManifestWriter) Close() error {
if w.closed {
- return nil
+ return w.closeErr
}
+ w.closed = true
+ var emptyErr error
if w.addedFiles+w.existingFiles+w.deletedFiles == 0 {
- return errors.New("empty manifest file has been written")
+ emptyErr = errors.New("empty manifest file has been written")
}
- w.closed = true
+ w.closeErr = errors.Join(emptyErr, w.writer.Close())
- return w.writer.Close()
+ return w.closeErr
Review Comment:
Now that `Close()` is terminal and returns the cached error on every call, I
want to check the public wrappers. `WriteManifest` and `WriteManifestV3` both
call `Close()` explicitly and also `defer internal.CheckedClose(w, &err)`, so
on an empty writer they'll join the returned error with a second `Close()`
result — `errors.Join(emptyErr, emptyErr)` — and hand the caller a doubled
`empty manifest file has been written\nempty manifest file has been written`.
If those paths are reachable with zero entries (they look like they are —
nothing guards the entry count before `WriteManifest`), I'd drop the redundant
deferred `CheckedClose` there since `Close()` is already checked explicitly, or
have `CheckedClose` skip an already-closed writer. Thoughts?
##########
manifest_test.go:
##########
@@ -2091,6 +2091,20 @@ func (m *ManifestTestSuite) TestManifestWriterMeta() {
m.Equal("[]", string(md["partition-spec"]))
}
+func (m *ManifestTestSuite) TestEmptyManifestWriterCloseIsTerminal() {
+ var out bytes.Buffer
+ writer, err := NewManifestWriter(2, &out, *UnpartitionedSpec,
testSchema, snapshotID)
+ m.Require().NoError(err)
+
+ firstErr := writer.Close()
Review Comment:
Good that this pins the terminal empty-close behavior. It only covers the
empty branch though — I'd add the non-empty happy path (close a writer with
entries, call `Close()` again, assert both return nil) so the new `closeErr`
field's nil case is locked in, plus a case where the underlying writer errors
on close so the two-error `errors.Join` path actually gets exercised.
Minor: `m.Equal(firstErr, writer.Close())` passes on pointer identity (both
calls hand back the same cached error), which is correct but implicit — a
one-line comment there would help the next reader.
--
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]
