kv: properly merge CollectedSpans for split BatchResponses #17414
Conversation
Previously we would only return the `CollectedSpans` of the last `BatchResponse` component for a given `BatchRequest` sent by `DistSender`. This bug was subtle and stayed hidden for two reasons: - `CollectedSpans` are only used for snowball tracing when sending remote RPCs, so the issue would never be seen in a single node deployment. - Very few `BatchRequest`s get split in practice, as we try hard to send batches using 1 phase commit wherever possible. The test added will fail for the `node-1` and `node-2` subtests without this fix, which demonstrates the existence of the bug.
|
This change is |
|
LGTM, thanks for the fix! It's a bit strange that there are two parts of dist-sender that deal with combining responses, and they do it differently ( Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, all commit checks successful. Comments from Reviewable |
|
@radu Combine() is used to reassemble a multi-range batch and so it's mostly concerned with combining the responses. It's similar but doesn't quite fit here. I agree the code should somehow be cleaner. Reviewed 3 of 3 files at r1. Comments from Reviewable |
all files reviewed at latest revision, all discussions resolved, all commit checks successful.
Previously we would only return the
CollectedSpansof the lastBatchResponsecomponent for a givenBatchRequestsent byDistSender. This bug was subtle and stayed hidden for two reasons:CollectedSpansare only used for snowball tracing when sendingremote RPCs, so the issue would never be seen in a single node
deployment.
BatchRequests get split in practice, as we try hardto send batches using 1 phase commit wherever possible.
The test added will fail for the
node-1andnode-2subtests withoutthis fix, which demonstrates the existence of the bug.