laskoviymishka commented on code in PR #1225:
URL: https://github.com/apache/iceberg-go/pull/1225#discussion_r3448957065
##########
table/rolling_data_writer.go:
##########
@@ -429,6 +428,8 @@ func (r *RollingDataWriter) closeAndWait() error {
return fmt.Errorf("error in rolling data writer: %w", err)
}
+ r.cancel()
Review Comment:
placing `cancel()` only on the success return means the error branch above
(`return fmt.Errorf("error in rolling data writer: %w", err)`) now exits
without ever cancelling — so every writer that hits a stream error leaks its
derived context until the parent is cancelled, which in a long-lived process
(compaction daemon, streaming ingest) may be never. `closeAll()` keeps
iterating after the first error, so a batch of failing writers leaks one each.
I'd move this further out: `defer r.cancel()` inside the `stream` goroutine.
It owns the derived ctx and every caller already `wg.Wait()`s on it, so cancel
fires once when streaming ends on any path — that also covers the
`unpartitionedWrite` sites in `arrow_utils.go` and `closeCurrentWriter` in
`clustered_writer.go`, which call `close()` directly and lose the cancel they
used to get from it. A `defer r.cancel()` here in `closeAndWait()` only fixes
this branch and leaves those two leaking. wdyt?
##########
table/rolling_data_writer.go:
##########
@@ -429,6 +428,8 @@ func (r *RollingDataWriter) closeAndWait() error {
return fmt.Errorf("error in rolling data writer: %w", err)
Review Comment:
minor: if `cancel()` ends up staying here rather than moving into the
goroutine, nlreturn will want a blank line before this return too — the new
`return nil` below got one but this branch didn't. CI runs gofumpt + nlreturn,
so worth a quick `make lint` before pushing.
--
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]
