fix: BatchWriter: Ensure flushed batch has at least one record#2489
fix: BatchWriter: Ensure flushed batch has at least one record#2489kodiakhq[bot] merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates BatchWriter’s worker loop to avoid flushing empty insert batches (i.e., calling WriteTableBatch when the current batch has 0 rows), addressing the issue linked in cloudquery/cloudquery#22750.
Changes:
- Guard the “flush current batch” path so
send()is only called when the current batch contains at least one row (limit.Rows() > 0).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Agent-Logs-Url: https://github.com/cloudquery/plugin-sdk/sessions/68c913bf-a3a7-4ceb-8d10-daeb3fcbb260 Co-authored-by: bbernays <30294676+bbernays@users.noreply.github.com>
disq
changed the title
| if limit.Rows() > 0 { | ||
| send() | ||
| } | ||
| ticker.Reset(w.batchTimeout) |
There was a problem hiding this comment.
Should this go inside the if as well?
There was a problem hiding this comment.
Or have batch.SliceRecord return toFlush = 0?
There was a problem hiding this comment.
I wanted to reset the batchTimeout even if there were no resources to ensure that we don't just sync a single row the next time
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| testClient := &noEmptyBatchClient{t: t} | ||
| // batchSizeBytes=1 ensures that no single row fits in the initial batch: | ||
| // SliceRecord returns add==nil, toFlush=[one record per row], rest=nil. |
There was a problem hiding this comment.
The comment describing the WithBatchSizeBytes(1) scenario says SliceRecord returns ... rest=nil, but internal/batch.SliceRecord currently returns a non-nil remaining when the byte cap is too small (because getToFlush slices to single-row batches without clearing the original record). Please update this comment to reflect the actual SliceRecord behavior so future readers aren’t misled about why this regression case triggers.
| // SliceRecord returns add==nil, toFlush=[one record per row], rest=nil. | |
| // SliceRecord returns add==nil, toFlush=[one record per row], and a non-nil remaining | |
| // record for the unsliced original batch. |
Summary
Ensure only flushing batches that have more than 0 records.
Issue originally was discovered in cloudquery/cloudquery#22750