tanmayrauth commented on code in PR #1595:
URL: https://github.com/apache/iceberg-go/pull/1595#discussion_r3670127182
##########
table/partitioned_fanout_writer.go:
##########
@@ -175,10 +180,11 @@ func (p *partitionedFanoutWriter) processRecord(ctx
context.Context, writerCtx c
return nil
}
-func (p *partitionedFanoutWriter) yieldDataFiles(fanoutWorkers
*errgroup.Group, outputDataFilesCh chan iceberg.DataFile, writerCancel
context.CancelFunc) iter.Seq2[iceberg.DataFile, error] {
+func (p *partitionedFanoutWriter) yieldDataFiles(fanoutWorkers
*errgroup.Group, inputRecordsCh chan arrow.RecordBatch, outputDataFilesCh chan
iceberg.DataFile, writerCancel context.CancelFunc) iter.Seq2[iceberg.DataFile,
error] {
Review Comment:
The param here is still named `writerCancel`, but it now receives the
combined `cancel` that stops both the fanout/feeder context and the writer
context. The old name understates that — a maintainer reading
`writerCancel` won't realize calling it also cancels record production,
which is the whole point of this change. Suggest renaming it to `cancel` to
match the renamed param on the shared `yieldDataFiles`
below.
##########
table/write_records_test.go:
##########
@@ -233,6 +234,63 @@ func (s *WriteRecordsTestSuite)
TestSmallTargetFileSizeProducesMultipleFiles() {
s.Equal(int64(1000), totalRows)
}
+func (s *WriteRecordsTestSuite) TestEarlyStopCancelsRecordProduction() {
Review Comment:
Nice regression. One gap: it covers unpartitioned + partitioned data writes
but not positionDeletePartitionedFanoutWriter, which took the identical
cancel/drain change. The shared
yieldDataFiles/startRecordFeeder path is exercised by the partitioned
case, so this is low priority — but its processBatch path is the one bit that
differs and currently has no early-stop coverage. A pos-delete variant of this
test would close that.
--
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]
