Make reply capture opt-in (--capture-replies) so it doesn't inflate query latency - #123
Merged
Merged
Conversation
) #114 changed the per-command receiver from a discarded nil interface to new(interface{}) so RxBytes could be populated. But that fully unmarshals every RESP reply into nested Go values via reflection INSIDE client.Do -- i.e. inside the measured latency window. For FT.SEARCH / FT.AGGREGATE (O(docs*fields) allocations per query) this materially inflates the very read/vector latencies these benchmarks exist to measure, and RxBytes has no consumer in the specs. Make capture opt-in via --capture-replies (default false): the receiver is a nil interface again, so radix reads-and-discards the reply with no allocation or reflection -- restoring latency fidelity by default. With the flag on, replies are decoded and getRxLen populates RxBytes. Command errors are surfaced by radix regardless of the receiver, so error accounting is unaffected either way. Validated E2E: default run -> RxBytes=0, TxBytes unchanged; --capture-replies -> RxBytes>0, same TxBytes. Adds TestFTSBCaptureRepliesControlsRxBytes and updates the pipeline-tail test to the new default.
|
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
1 participant
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 #117.
Problem
#114 changed the per-command receiver from a discarded nil
interface{}tonew(interface{})soRxBytescould be populated. But a*interface{}receiver makes radix fully unmarshal every RESP reply into nested Go values via reflection — insideclient.Do, i.e. inside the measured latency window. ForFT.SEARCH/FT.AGGREGATE(O(docs × fields) allocations per query) this materially inflates the very read/vector latencies these benchmarks exist to measure, and no RediSearch benchmark spec consumesRxBytes.Fix
Make reply capture opt-in via
--capture-replies(defaultfalse):RxBytesis 0.--capture-replies: replies are decoded andgetRxLenpopulatesRxBytes.Command errors are surfaced by radix regardless of the receiver, so error accounting is unaffected either way.
Validation
E2E (2000 HSET):
--capture-repliesTxBytes(sent-byte accounting) is identical both ways. AddsTestFTSBCaptureRepliesControlsRxBytes(asserts the flag flips RxBytes 0↔>0) and updates the pipeline-tail test to the new default. Unit + integration suites green (-race).