perf: flush completion-tracked items on idle to bound Ack latency#20
Merged
Conversation
A lone IngestSync/IngestBatch item previously waited up to maxDelay (1s default) for the flush ticker before its first POST, so that interval was added synchronously to a pub/sub handler's Ack latency. The worker now flushes as soon as the channel goes idle whenever the buffer holds a completion-tracked item (ack != nil) — those callers are blocked waiting to Ack, so they are latency sensitive by nature. Before flushing it greedily drains whatever is already queued (up to batchSize), so a burst still coalesces into batches and a bulk IngestBatch still fills full batches; idle flush only kicks in when items genuinely aren't available yet. The behavior is gated on a tracked item being present, so pure fire-and-forget traffic never triggers it and its batching is unchanged (only an extra ack != nil check per item on the hot path). The trade for tracked traffic is more, smaller requests at low/moderate volume in exchange for round-trip latency instead of maxDelay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #19.
Problem
A lone
IngestSync/IngestBatchitem waited up tomaxDelay(1s default) for the flush ticker before its first POST, so that interval was added synchronously to a pub/sub handler's Ack latency. The scrutinize pass on #19 flagged this as the one cost that genuinely matters at low volume.Change
The worker now flushes as soon as the channel goes idle whenever the buffer holds a completion-tracked item (
ack != nil). Those callers are blocked waiting to Ack, so they're latency-sensitive by nature. Before flushing it greedily drains whatever is already queued (up tobatchSize), so:IngestSynccalls still coalesces into batches,IngestBatch(N)still fills fullbatchSizebatches (the items are already queued when the drain runs),Result: Ack latency is bounded by the round-trip instead of
maxDelay.Why no new API
It's gated on a tracked item being present (
hasTracked), so pure fire-and-forget traffic never triggers it — its batching is byte-for-byte unchanged (only an extraack != nilbool check per item on the hot path). No knob to set, self-tuning. The trade for tracked traffic is more, smaller requests at low/moderate volume in exchange for round-trip latency rather thanmaxDelay— which is the right trade for a caller blocking on durability.Tests
3 new (
ingest_sync_test.go): tracked item returns well undermaxDelay; fire-and-forget still waits for the timer (guards the unchanged path); a tracked burst still delivers every record once, in order. Full suite green,go vetclean, race-clean across repeated/stress runs (timing-sensitive tests held over-count=5 -race).🤖 Generated with Claude Code