Re: [PR] fix(manifest): make empty writer close terminal [iceberg-go]

["laskoviymishka (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org