Fix --pipeline >1 panic and pipelined byte/label accounting - #114
Merged
Conversation
…ipeline >1 panic
Buffer each pipelined command as a pendingCmd carrying its own send time, sent-byte
count, reply receiver, and labels, instead of threading parallel scalar slices
through the flush. This fixes several defects on the --pipeline N>1 path (default
pipeline=1 was already correct):
- Panic: the old code kept a single `replies` slice that connectionProcessor
never grew, while cmds/times accumulated to `pipeline`; the flush indexed
`replies[pos]` out of range -> "index out of range [1] with length 1", crashing
the worker. --pipeline >1 was unusable. Now each command owns its receiver.
- Per-command TxBytes: the flush recorded one scalar txBytesCount for every
command; now each records its own sent bytes.
- Per-command labels: cmdType/cmdQueryId are now per-command, so a mixed pipeline
no longer files every command under the flushing command's label.
- Reply capture: sendFlatCmd now passes a *interface{} receiver (was a nil
interface, so replies were discarded and RxBytes was always 0). getRxLen sizes
the captured reply (string/[]byte/[]interface{}/int64, deref *interface{}).
- Fix READ_CURSOR label: the histogram switch matched "CURSOR_READ" but the code
emits "READ_CURSOR", so cursor reads were dropped from readCursorHistogram.
Validated E2E against redis: master crashes at --pipeline 4 (index out of range,
8/2000 keys); this branch completes (2000/2000 keys) with TxBytes=135780 and
RxBytes=2000. At pipeline=1 the Tx/ops numbers are byte-identical to master.
Closes #113
The refactor collapsed the timeout log message to a bare count, dropping the
command and key. That broke integration tests (TestFTSBWithTimeout,
TestFTSBWithLogFileAndTimeout) which assert the log contains 'Timeout occurred'
+ the command ('DEBUG') + key ('SLEEP') on one line. Restore the per-command
detail using the representative pending command.
|
fcostaoliveira
added a commit
that referenced
this pull request
Jul 13, 2026
…ne latency Supersedes the earlier latency-only change on this branch. The 7-way adversarial review showed the original premise was wrong: hdrhistogram-go v1.0.1 does NOT drop RecordValue(0) (it is counted), so the latency clamp never prevented any TotalOps undercount. The real op-drops were elsewhere. This commit fixes them: A. Flush the trailing partial pipeline window. connectionProcessor buffered up to `pipeline` commands per slot but never flushed the leftover (< pipeline) at end of input, so the last `rows % pipeline` commands per batch were NEVER sent to Redis or counted -- silent data loss whenever pipeline did not divide the row count (e.g. pipeline=3 over 100 rows -> 99 sent). sendIfRequired is split into a guard + reusable flushPending; connectionProcessor now flushes every non-empty slot before finishing. B. Make TotalOps/overallOpsRate exact via an atomic counter (one increment per recorded command) instead of the HDR histograms' TotalCount(), which drops any latency above the trackable cap (--max-latency-seconds, ~1.05s default -- a cold query or any i/o timeout) and loses increments under concurrent unlocked RecordValue. The histograms remain the source for percentiles only. C. Keep the send-time uniform pipeline latency (one batch round-trip attributed to each command; matches redis-benchmark) but reframe the 1us floor honestly: a network round-trip is never 0us, so a 0 only reflects timer resolution -- it is NOT a TotalOps fix. Also capture sendT after building the pipeline action so client-side slice bookkeeping is not charged as latency. Validated E2E: TotalOps == dbsize == input count at pipeline 1/3/4/8/16/100 (was 99/1980/1920 before). Adds: atomic-counter decoupling unit test, a pipeline=3 integration test asserting exact TotalOps, and reframed latency test. Follow-ups filed separately: histogram RecordValue data race under workers>1; #114 reply-capture unmarshal inflating FT.SEARCH latency.
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.



Closes #113.
Problem
On the
--pipeline N(N>1) path,sendFlatCmd/sendIfRequired/connectionProcessorthreaded parallel scalar values through the flush, which caused several defects (the defaultpipeline=1path was already correct):connectionProcessorreassignedcmds/timesfromsendFlatCmd(so they accumulated topipeline) butreplieswas never grown, so the flush loop indexedreplies[pos]out of range →index out of range [1] with length 1, crashing the worker goroutine.txBytesCountfor every command in the batch, soTxBytesbecameΣ(pipeline × flushing-row bytes)instead of the sum of each command's bytes.cmdType/cmdQueryIdwere per-flush scalars, so a mixed pipeline filed every command under the flushing command's label (wrong histogram grouping).sendFlatCmdpassed a nilinterface{}receiver toradix.Cmd, so replies were discarded andgetRxLenalways saw nil. (Itsstring/[]stringbranches were dead.)READ_CURSORlabel mismatch. The histogram switch matchedcase "CURSOR_READ"while the code emitsREAD_CURSOR, so cursor reads were dropped fromreadCursorHistogram.Fix
Buffer each pipelined command as a
pendingCmdthat carries its own send time, sent-byte count, reply receiver, and labels. The flush then records one correct stat per command. Specifically:[]pendingCmdbuffer per slot inconnectionProcessor(no separaterepliesslice to fall out of sync).sendFlatCmdpasses a*interface{}receiver so replies are captured;getRxLensizesstring/[]byte/[]interface{}(recursive)/int64, dereferencing*interface{}.txBytes, latency,cmdType, andcmdQueryId.benchmark_runnerhistogram switch corrected tocase "READ_CURSOR".Validation (E2E against redis)
--pipeline 4TxBytesRxBytesAt
pipeline=1,TotalOpsandTxBytesare byte-identical to master (2000 / 135780);RxBytesgoes from 0 → 2000 (replies now counted).Tests
TestPipelineRecordsPerCommandTxAndDoesNotPanic— withpipeline=2, two buffered commands flush without panicking and record their own Tx ([100, 200]; the old code produced[200, 200]and, in the real call path, panicked).TestGetRxLenextended for[]byte/[]interface{}/int64/*interface{}.sendFlatCmdsignature and still pass.Notes / out of scope
*interface{}receiver allocates per command (radix previously discarded replies). This restores the evidently-intended behavior (getRxLenpredates this PR) and makesRxBytesmeaningful. Negligible for the dominant ingest path (integer/OK replies); for large query replies it is more — can be gated behind a flag in a follow-up if it perturbs latency measurement.TotalOps1-off at pipeline>1: pipelined commands share one completion time, so the last has ~0µs measured latency, which the HDR histogram (min=1) rejects — a pre-existing pipeline latency-accounting semantics issue, not introduced here (master crashed before it was observable).