Re: [PR] fix: remature context cancellation during graceful shutdown in PartitionedFanoutWriter [iceberg-go]

["laskoviymishka (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org