zeroshade commented on code in PR #1610:
URL: https://github.com/apache/iceberg-go/pull/1610#discussion_r3732158566
##########
manifest.go:
##########
@@ -1358,16 +1359,30 @@ 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")
Review Comment:
Consider promoting this to an exported sentinel — `var ErrEmptyManifest =
errors.New("empty manifest file has been written")` — and wrapping it here. As
an inline `errors.New`, callers have no way to `errors.Is` the condition, and
the new tests have to match on the message text. Given that this PR is
precisely about making the empty-manifest error stable and observable across
repeat closes, a sentinel feels like the natural companion. Non-blocking.
##########
manifest_test.go:
##########
@@ -2091,6 +2108,117 @@ func (m *ManifestTestSuite) TestManifestWriterMeta() {
m.Equal("[]", string(md["partition-spec"]))
}
+func (m *ManifestTestSuite) TestEmptyManifestWriterCloseIsTerminal() {
Review Comment:
The central question this change settles — that the file left on the output
is now a readable Avro file containing zero entries — isn't asserted anywhere.
Worth extending this case to decode `out.Bytes()` back and assert an empty
entry list. That readability property is exactly what distinguishes the new
behavior from the pre-PR truncated output, so it seems worth pinning.
Suggestion only; the terminal-close assertions here are already solid.
##########
manifest.go:
##########
@@ -1753,7 +1768,11 @@ func WriteManifest(
if err != nil {
return nil, err
}
- defer internal.CheckedClose(w, &err)
+ defer func() {
Review Comment:
Non-blocking, for consistency: this `if !w.closed` guard (and its twin in
`WriteManifestV3`) is the right pattern, but it is applied only at these two
call sites. The `defer internal.CheckedClose(wr, &retErr)` plus explicit
`wr.Close()` pairs in `table/snapshot_producers.go` (`:223`, `:378`, `:837`,
`:907`) will now join the cached error with itself and surface a doubled
message. Worth a follow-up that either repeats the guard there or moves the
once-only behavior into a helper on the writer, so callers don't each have to
know about it.
##########
manifest_test.go:
##########
@@ -2091,6 +2108,117 @@ 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()
+ m.Require().EqualError(firstErr, "empty manifest file has been written")
+ m.ErrorContains(writer.Add(manifestEntryV2Records[0]), "closed manifest
writer")
+ m.Equal(firstErr, writer.Close())
+
+ _, err = writer.ToManifestFile("manifest.avro", int64(out.Len()))
+ m.Equal(firstErr, err)
+}
+
+func (m *ManifestTestSuite) TestManifestWriterSuccessfulCloseIsTerminal() {
+ var out bytes.Buffer
+ writer, err := NewManifestWriter(2, &out, *UnpartitionedSpec,
testSchema, snapshotID)
+ m.Require().NoError(err)
+ m.Require().NoError(writer.Add(manifestEntryV2Records[0]))
+
+ m.NoError(writer.Close())
+ m.NoError(writer.Close())
+}
+
+func (m *ManifestTestSuite)
TestEmptyManifestWriterCloseAfterConstructionFailure() {
Review Comment:
Consider a companion case: `Close()` after an `addEntry` failure with
entries already written. That exercises the branch where `emptyErr` is nil but
`writerErr` is not, and confirms the cached result stays stable across repeat
closes on that path too. Non-blocking.
##########
manifest_test.go:
##########
@@ -2091,6 +2108,117 @@ 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()
+ m.Require().EqualError(firstErr, "empty manifest file has been written")
+ m.ErrorContains(writer.Add(manifestEntryV2Records[0]), "closed manifest
writer")
+ m.Equal(firstErr, writer.Close())
+
+ _, err = writer.ToManifestFile("manifest.avro", int64(out.Len()))
+ m.Equal(firstErr, err)
+}
+
+func (m *ManifestTestSuite) TestManifestWriterSuccessfulCloseIsTerminal() {
+ var out bytes.Buffer
+ writer, err := NewManifestWriter(2, &out, *UnpartitionedSpec,
testSchema, snapshotID)
+ m.Require().NoError(err)
+ m.Require().NoError(writer.Add(manifestEntryV2Records[0]))
+
+ m.NoError(writer.Close())
+ m.NoError(writer.Close())
+}
+
+func (m *ManifestTestSuite)
TestEmptyManifestWriterCloseAfterConstructionFailure() {
+ writeErr := errors.New("write failed")
+
+ writer, err := NewManifestWriter(
+ 2,
+ manifestFailingWriter{err: writeErr},
+ *UnpartitionedSpec,
+ testSchema,
+ snapshotID,
+ )
+ m.Require().ErrorIs(err, writeErr)
+ m.Require().NotNil(writer)
+
+ var closeErr error
+ m.NotPanics(func() {
+ closeErr = writer.Close()
+ })
+ m.EqualError(closeErr, "empty manifest file has been written")
+
+ // Close returns the cached result after becoming terminal.
+ m.Equal(closeErr, writer.Close())
+}
+
+func (m *ManifestTestSuite) TestEmptyManifestWriterCloseJoinsUnderlyingError()
{
+ writer, err := NewManifestWriter(
+ 2,
+ io.Discard,
+ *UnpartitionedSpec,
+ testSchema,
+ snapshotID,
+ )
+ m.Require().NoError(err)
+
+ avroSchema := writer.writer.Schema()
+ m.Require().NoError(writer.writer.Close())
+
+ underlyingErr := errors.New("underlying close failed")
+ writer.writer, err = ocf.NewWriter(
+ io.Discard,
+ avroSchema,
+ ocf.WithCodec(manifestCloseErrorCodec{
+ Codec: ocf.DeflateCodec(flate.DefaultCompression),
+ err: underlyingErr,
+ }),
+ )
+ m.Require().NoError(err)
+
+ firstErr := writer.Close()
+ m.Require().Error(firstErr)
+ m.ErrorIs(firstErr, underlyingErr)
+ m.EqualError(
+ firstErr,
+ "empty manifest file has been written\nunderlying close failed",
+ )
+
+ // Close returns the cached combined result after becoming terminal.
+ m.Equal(firstErr, writer.Close())
+}
+
+func (m *ManifestTestSuite) TestWriteManifestEmptyErrorIsNotDuplicated() {
Review Comment:
Non-blocking coverage note: all the new cases construct v2 writers. Since
`Close()` is shared across versions and `WriteManifestV3` received the same
guard, running these table-driven over v1/v2/v3 would be cheap insurance
against a version-specific regression.
--
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]
