alexandre-normand commented on code in PR #1673:
URL: https://github.com/apache/iceberg-go/pull/1673#discussion_r3731151361
##########
table/snapshot_producers.go:
##########
@@ -1328,6 +1364,12 @@ func (sp *snapshotProducer)
commitManifests(newManifests, addedContent []iceberg
return nil, fmt.Errorf("rebuild manifest list:
write: %w", wErr)
}
}
+ if closeErr := closeManifestListOutput(fio, manifestListPath,
out, writer, false); closeErr != nil {
+ closed = true
+
+ return nil, fmt.Errorf("rebuild manifest list: close:
%w", closeErr)
+ }
+ closed = true
Review Comment:
Maybe it's just me missing nuances and being naive but this seems like a lot
of code. Why can't we just have the writer's closing be done after the `write`
operation and return the error if there's one? After all, the contract with
blob storage is essentially that a write is only guaranteed to be successful
when `Close` has returned without an error.
If _that_ approach worked, it seems like we could extend the same pattern to
other paths that are prone to the same flaw/bug. I _think_ that some of those
other paths would *not* have the same impact/severity nor would they all lead
to corrupted snapshots so this might be more just about using the blob storage
API like it's been intended to but it would help to promote better hygiene?
--
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]
