laskoviymishka commented on code in PR #1589:
URL: https://github.com/apache/iceberg-go/pull/1589#discussion_r3680747497
##########
table/dv/dv_writer.go:
##########
@@ -260,6 +256,10 @@ func (w *DVWriter) Flush(_ context.Context, location
string) ([]iceberg.DataFile
dataFiles = append(dataFiles, df)
}
+ if err := w.fs.WriteFile(location, fileBytes); err != nil {
Review Comment:
Not blocking and it's pre-existing, but since this PR is squarely about the
error path: if this `WriteFile` fails we return without resetting
`w.entries`/`w.order`, so a caller that retries `Flush` re-serializes and
writes again — and if the bytes actually landed but the ACK was lost, the retry
lands a second copy at a fresh location and orphans the first.
The reorder doesn't make this worse, but it feels like the right moment to
either reset state after a successful write or document on `Flush` that it
isn't safe to retry after a write failure. wdyt?
##########
table/dv/dv_writer_test.go:
##########
@@ -452,13 +453,17 @@ func TestDVWriterFlushMixedSpecIDs(t *testing.T) {
func TestDVWriterFlushUnknownSpecID(t *testing.T) {
fs := newTestFS()
w := NewDVWriter(fs, unpartitionedResolver())
+ location := "mem://test/unknown-spec.puffin"
// specID 99 is not registered with the resolver.
require.NoError(t, w.Add("s3://bucket/file.parquet", []int64{1}, 99,
nil))
- _, err := w.Flush(context.Background(),
"mem://test/unknown-spec.puffin")
+ _, err := w.Flush(context.Background(), location)
require.Error(t, err)
assert.Contains(t, err.Error(), "unknown partition spec id 99")
+
+ _, openErr := fs.Open(location)
+ assert.ErrorIs(t, openErr, stdfs.ErrNotExist)
Review Comment:
tiny one: this `ErrorIs` is the actual point of the test — proving no file
was written — so I'd make it `require.ErrorIs` to match the `require.Error`
above. Not a blocker.
##########
table/dv/dv_writer_test.go:
##########
@@ -452,13 +453,17 @@ func TestDVWriterFlushMixedSpecIDs(t *testing.T) {
func TestDVWriterFlushUnknownSpecID(t *testing.T) {
fs := newTestFS()
w := NewDVWriter(fs, unpartitionedResolver())
+ location := "mem://test/unknown-spec.puffin"
// specID 99 is not registered with the resolver.
require.NoError(t, w.Add("s3://bucket/file.parquet", []int64{1}, 99,
nil))
Review Comment:
Nice regression test — this does fail on the pre-fix ordering. Since the PR
description calls out "mid-loop failure across multiple entries leaves nothing
behind," I'd add a second entry with a known spec ahead of the unknown one, so
we also prove a partial metadata loop leaves no file behind. Not 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]
