Open erikgrinaker opened 4 months ago
Hi @erikgrinaker, please add branch-* labels to identify which branch(es) this release-blocker affects.
:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.
The sync.Map
was addressed in #120597. If we make it generic in #120604, it might save some allocations when creating circuit breakers in ForReplica()
.
At this point, circuit breakers don't appear to have a significant effect on end-to-end benchmarks, so they may already be "good enough".
Some known optimization opportunities:
sync.Map
.newReplicaCircuitBreaker
for ForReplica()
involves 16 allocations.Breaker.Signal().Err()
.
https://github.com/cockroachdb/cockroach/blob/91e72787579190e4b4c4bcbc95d1017ccb8c8808/pkg/kv/kvclient/kvcoord/dist_sender_circuit_breaker.go#L504-L506Removing this from blocking the release, but https://github.com/cockroachdb/cockroach/issues/121206 is still a GA-blocker.
The DistSender circuit breakers added in https://github.com/cockroachdb/cockroach/pull/118943 need additional benchmarking and performance testing, particularly at large scale and high concurrency. We should consider using a
sync.Map
orIntMap
variant, look at the effect of large numbers of probes (e.g. when a high-density node's disk stalls), consider batching probes, and use async.Pool
for circuit breaker construction.Jira issue: CRDB-36388