laskoviymishka commented on code in PR #1594:
URL: https://github.com/apache/iceberg-go/pull/1594#discussion_r3681123568
##########
puffin/puffin_test.go:
##########
@@ -52,6 +72,62 @@ func defaultBlobInput() puffin.BlobMetadataInput {
}
}
+func TestWriterRejectsOperationsAfterPartialWriteFailure(t *testing.T) {
+ tests := []struct {
+ name string
+ failCall int
+ }{
+ {name: "blob", failCall: 2},
Review Comment:
These `failCall` numbers encode the global ordinal of each write across
NewWriter, AddBlob, and Finish — 2 is the blob write, 3–5 the footer writes.
Nothing ties them to the site they're meant to hit, though: add a checksum or a
codec byte anywhere in AddBlob and every index below it shifts by one, the `if
tt.failCall == 2` branch at line 94 silently misclassifies, and the suite stays
green while intercepting the wrong call.
I'd swap the raw number for something that names the intent — a
`failsDuringAdd bool` on the table (true only for the blob case) driving that
branch, plus a short comment enumerating which call each index is. I mostly
want the drift to be loud instead of silent. wdyt?
##########
puffin/puffin_writer.go:
##########
@@ -53,6 +53,7 @@ type Writer struct {
blobs []BlobMetadata
props map[string]string
done bool
+ failed error
Review Comment:
This `failed` field is a real behavioral contract — once any physical write
errors, every subsequent call is rejected — and it isn't written down anywhere.
I think poisoning on failure is the right instinct (the stream's already
half-written, the file is toast), but it's worth a line on the Writer type or
the AddBlob/Finish godoc so callers know a failed writer is dead and not
retryable.
One thing to be deliberate about: AddProperties, ClearProperties, and
SetCreatedBy get poisoned too, and none of them touch the stream. Blocking them
is defensible — the writer's tainted either way — but it'll surprise someone
who expects only the byte-writing methods to fail, and it diverges from Java's
PuffinWriter, which has no failed state at all. I'd keep the poison broad and
just document that it's intentional.
##########
puffin/puffin_test.go:
##########
@@ -52,6 +72,62 @@ func defaultBlobInput() puffin.BlobMetadataInput {
}
}
+func TestWriterRejectsOperationsAfterPartialWriteFailure(t *testing.T) {
+ tests := []struct {
+ name string
+ failCall int
+ }{
+ {name: "blob", failCall: 2},
+ {name: "footer magic", failCall: 3},
+ {name: "footer payload", failCall: 4},
+ {name: "footer trailer", failCall: 5},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ writeErr := errors.New("injected partial write")
+ output := &partialFailWriter{failCall: tt.failCall,
err: writeErr}
+ writer, err := puffin.NewWriter(output)
+ require.NoError(t, err)
+
+ _, err = writer.AddBlob(defaultBlobInput(),
[]byte("first blob payload"))
+ if tt.failCall == 2 {
+ require.ErrorIs(t, err, writeErr)
+ } else {
+ require.NoError(t, err)
+ require.ErrorIs(t, writer.Finish(), writeErr)
+ }
+
+ callsAfterFailure := output.calls
+ bytesAfterFailure := output.Len()
+
+ _, err = writer.AddBlob(defaultBlobInput(),
[]byte("second blob payload"))
+ assert.ErrorIs(t, err, writeErr)
+ assert.ErrorIs(t, writer.Finish(), writeErr)
+ assert.ErrorIs(t,
writer.AddProperties(map[string]string{"key": "value"}), writeErr)
+ assert.ErrorIs(t, writer.ClearProperties(), writeErr)
+ assert.ErrorIs(t, writer.SetCreatedBy("test"), writeErr)
+ assert.Equal(t, callsAfterFailure, output.calls)
+ assert.Equal(t, bytesAfterFailure, output.Len())
+ })
+ }
+}
+
+func TestWriterRejectsOperationsAfterShortWrite(t *testing.T) {
+ output := &partialFailWriter{failCall: 2}
+ writer, err := puffin.NewWriter(output)
+ require.NoError(t, err)
+
+ _, err = writer.AddBlob(defaultBlobInput(), []byte("first blob
payload"))
+ require.ErrorContains(t, err, "short write")
+
+ callsAfterFailure := output.calls
+ _, err = writer.AddBlob(defaultBlobInput(), []byte("second blob
payload"))
+ assert.ErrorContains(t, err, "short write")
+ assert.ErrorContains(t, writer.Finish(), "short write")
Review Comment:
This test only checks AddBlob and Finish get rejected, but the partial-write
test above asserts all five methods including the three property setters. Same
code path, so no real risk — just mirror the
AddProperties/ClearProperties/SetCreatedBy asserts here so the two stay
symmetric.
##########
puffin/puffin_writer.go:
##########
@@ -89,6 +90,9 @@ func NewWriter(w io.Writer) (*Writer, error) {
// SetProperties merges the provided properties into the file-level properties
// written to the footer. Can be called multiple times before Finish.
func (w *Writer) AddProperties(props map[string]string) error {
+ if w.failed != nil {
Review Comment:
Tiny thing that recurs in all five methods: `failed` is checked before
`done`. They can't both be set, so there's no bug — but it reads as though
`failed` outranks `finalized`, which is backwards. I'd check `done` first.
Non-blocking.
--
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]
