Skip to content

Commit 405a08c

Browse files
committed
kvcoord: remove call to Verify on responses
This check consumes CPU for no good reason. Network protocol (TCP) has a checksum, also, lower layers (storage) verifies the checksum as well. Sysbench-settings shows ~2% throughput increase: │ before.txt │ after.txt │ │ queries/sec │ queries/sec vs base │ SysbenchSettings/a=oltp_read_write/nodes=3/cpu=8/conc=64 29.97k ± 2% 30.61k ± 2% +2.16% (p=0.007 n=20) │ before.txt │ after.txt │ │ txns/sec │ txns/sec vs base │ SysbenchSettings/a=oltp_read_write/nodes=3/cpu=8/conc=64 1.498k ± 2% 1.531k ± 2% +2.16% (p=0.007 n=20) │ before.txt │ after.txt │ │ ms/min │ ms/min vs base │ SysbenchSettings/a=oltp_read_write/nodes=3/cpu=8/conc=64 12.56 ± 6% 11.78 ± 1% -6.21% (p=0.001 n=20) │ before.txt │ after.txt │ │ ms/avg │ ms/avg vs base │ SysbenchSettings/a=oltp_read_write/nodes=3/cpu=8/conc=64 42.72 ± 2% 41.80 ± 2% -2.13% (p=0.007 n=20) │ before.txt │ after.txt │ │ ms/p95 │ ms/p95 vs base │ SysbenchSettings/a=oltp_read_write/nodes=3/cpu=8/conc=64 65.65 ± 2% 64.47 ± 2% -1.80% (p=0.046 n=20) │ before.txt │ after.txt │ │ ms/max │ ms/max vs base │ SysbenchSettings/a=oltp_read_write/nodes=3/cpu=8/conc=64 238.4 ± 15% 228.2 ± 23% ~ (p=0.738 n=20) Fixes: #145541 Release note: None
1 parent bbe6bc6 commit 405a08c

File tree

1 file changed

+18
-8
lines changed

1 file changed

+18
-8
lines changed

pkg/kv/kvclient/kvcoord/transport.go

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/rpc"
1818
"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
1919
"github.com/cockroachdb/cockroach/pkg/util"
20+
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
2021
"github.com/cockroachdb/cockroach/pkg/util/log"
2122
"github.com/cockroachdb/cockroach/pkg/util/tracing"
2223
"github.com/cockroachdb/errors"
@@ -208,14 +209,23 @@ func (gt *grpcTransport) sendBatch(
208209
log.VEvent(ctx, 2, "sending batch request")
209210
reply, err := iface.Batch(ctx, ba)
210211
log.VEvent(ctx, 2, "received batch response")
211-
// If we queried a remote node, perform extra validation.
212-
if reply != nil && !rpc.IsLocal(iface) {
213-
if err == nil {
214-
for i := range reply.Responses {
215-
err = reply.Responses[i].GetInner().Verify(ba.Requests[i].GetInner())
216-
if err != nil {
217-
log.Errorf(ctx, "verification of response for %s failed: %v", ba.Requests[i].GetInner(), err)
218-
break
212+
213+
// We don't have any strong reason to keep verifying the checksum of the
214+
// response. However, since this check has historically caught some bugs, we
215+
// are keeping it in Test builds for not.
216+
// TODO(ibrahim): There is a path to remove Value checksum computations and
217+
// verifications. More details are available in:
218+
// https://github.com/cockroachdb/cockroach/issues/145541#issuecomment-2917225539
219+
if buildutil.CrdbTestBuild {
220+
// If we queried a remote node, perform extra validation.
221+
if reply != nil && !rpc.IsLocal(iface) {
222+
if err == nil {
223+
for i := range reply.Responses {
224+
err = reply.Responses[i].GetInner().Verify(ba.Requests[i].GetInner())
225+
if err != nil {
226+
log.Errorf(ctx, "verification of response for %s failed: %v", ba.Requests[i].GetInner(), err)
227+
break
228+
}
219229
}
220230
}
221231
}

0 commit comments

Comments
 (0)