Fix pipeline command accounting: flush trailing window + exact TotalOps - #115
Merged
Conversation
…m TotalOps Latency was computed as endT - pc.start, where pc.start is when a command was buffered into the pipeline window. In a pipeline the last-buffered command is flushed immediately, so its took rounded to 0us; the HDR histograms are New(1, cap, 3) (min 1us), so RecordValue(0) is rejected and that command silently vanishes from TotalOps and the latency histograms (observed: TotalOps=1999 for 2000 commands at --pipeline 4). It also skewed per-command latency: the first-buffered command's latency wrongly included the client-side wait for the window to fill. Measure latency from the batch send time instead (captured just before client.Do) and attribute that single round-trip to every command in the flush, clamped to >=1us. A pipeline is one client round-trip, so this is the correct client-observed latency and it can't be sub-microsecond over TCP. - No continuity impact: --pipeline >1 crashed before #114 (no historical data), and at pipeline=1 sendT equals the command's buffer time, so latency is unchanged. - Validated E2E: TotalOps == 2000 at --pipeline 1, 4, and 10 (was 1999 at 4). - Test asserts pipelined commands share one clamped, non-zero latency.
…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.
Contributor
Author
…ity gate - Extract the pipeline-flush error/timeout logging into logFlushError, dropping flushPending's cognitive complexity below the threshold (behavior unchanged; timeout integration tests still pass). - Extract the duplicated docker-redis setup in the integration tests into a shared startRedisContainer(t) helper (removes the new-code duplication the Sonar gate flagged; all 7 call sites now share one copy).
…cation Collapse the duplicated 'run binary + read json result' boilerplate in the JSON-asserting integration tests into a shared runFTSBReadJSON helper (drops the vestigial REDIS_URL env, which ftsb never reads). Pushes new-code duplication below the Sonar gate threshold.
- Collapse two more docker-setup blocks (blank-line variant missed earlier) into startRedisContainer. - Extract BenchmarkRunner.overallCounts() so GetOverallRatesMap and summary share the per-label count + atomic-totalOps gathering instead of duplicating it. Behavior unchanged; TotalOps/overallOpsRate verified E2E.
…rd, live-total reconcile Fixes from the pre-merge adversarial review (no change to the A/B/C behavior): - logFlushError: restore the historical "Fatal error with" prefix on the -continue-on-error=false path (the refactor had softened it to "Received an error with"). Control flow/exit unchanged; log text now matches master. - GetMeasuredRatiosMap: guard totalOps==0 so empty input yields 0 ratios instead of NaN (NaN is unmarshalable -> json.Marshal aborted the run with no result file). Adds a unit test. Also documents that ratios (histogram numerators over the exact atomic total) may sum to <1.0 on tail-heavy runs -- intended. - Live periodic reporter: derive its total ops from the atomic counter so the progress line reconciles with the final TotalOps/overallOpsRate. - Extract flooredMicros() with a deterministic unit test (the fake-client clamp assertion was timing-dependent). - GetTotalsMap: read TxBytes/RxBytes via atomic.LoadUint64 for consistency. - Move the flushPending doc comment back onto flushPending (the logFlushError extraction had detached it). - Integration test: assert TxBytes>0 and RxBytes>0 (reply capture) on the pipeline-tail run. Validated: empty input no longer crashes (exit 0, JSON written, ratios 0); pipeline-tail + timeout integration tests pass; full suite -race clean.
|
Contributor
Author
|
16-way adversarial review (Opus 4.8) done on the current HEAD — verdict: merge. Note for consumers: |
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.



Makes
ftsb_redisearchreport the correct command count and latency in pipeline mode. Follow-up to #114 (which made--pipeline >1usable).Problems (all on the
--pipeline N>1path;pipeline=1was already correct)connectionProcessorbuffered up topipelinecommands per slot but never flushed the leftover (< pipeline) at end of input, so the lastrows % pipelinecommands per batch were never sent to Redis or counted. E.g.--pipeline 3over a 100-row input → only 99 executed and counted. Verified:dbsizedropped too.TotalOps/overallOpsRatewere histogram-derived and lossy. They came fromhdrhistogram.TotalCount(), which (a) rejects any latency above the trackable cap (--max-latency-seconds, ~1.05s default — a coldFT.SEARCH, GC pause, or any i/o timeout), and (b) loses increments under concurrent unlockedRecordValue. Either silently undercounts ops.Fix
sendIfRequiredinto a guard + reusableflushPending, and flush every non-empty slot inconnectionProcessorbefore finishing. No command is left unsent.totalOpscounter (one increment per recorded command) and use it forTotalOps/overallOpsRate. The histograms now own percentiles only, where cap-clamping the tail is standard.redis-benchmark), capturesendTafter building the pipeline action, and reframe the 1µs floor honestly (a network round-trip is never 0µs; it is not a TotalOps fix).Validation (E2E against redis)
TotalOps == dbsize == input countat every pipeline depth, was lossy before:--pipelineTests
TestTotalOpsIsAtomicNotHistogramDerived— records an above-cap latency (rejected by the histogram) and assertsTotalOpsstill counts it (via the atomic counter).TestFTSBPipelineTailIsFlushedAndCounted— integration test:--pipeline 3over the 100-rowminimal.csvassertsTotalOps == 100(pre-fix: 99). Runs in CI's Docker integration step.TestPipelineRecordsPerCommandTxAndDoesNotPanic— per-command Tx + equal/floored batch latency.cmd/...+benchmark_runnersuites pass with-race.Out of scope — filed as follow-ups
RecordValuedata race under-workers >1(corrupts percentiles): see the tracking issue.FT.SEARCHlatency: see the tracking issue.