kvflowcontrol: squash data race#104855
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)
pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go line 110 at r1 (raw file):
// the mutex. To avoid the data race, copy out the pointers first. connections := make([]*connectedStream, len(h.mu.connections)) copy(connections, h.mu.connections)
Admit is on a performance critical path. I think we need a copy-on-write scheme instead of copy-on-read.
Fixes cockroachdb#104837. Fixes cockroachdb#105762. Release note: None
8ac33ee to
edd1e1b
Compare
irfansharif
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go line 110 at r1 (raw file):
Previously, sumeerbhola wrote…
Admit is on a performance critical path. I think we need a copy-on-write scheme instead of copy-on-read.
Done.
bananabrick
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go line 239 at r2 (raw file):
func (h *Handle) connectStreamLocked( ctx context.Context, pos kvflowcontrolpb.RaftLogPosition, stream kvflowcontrol.Stream,
Does each connection have a unique Stream?
irfansharif
left a comment
There was a problem hiding this comment.
TFTR.
bors r+
Reviewable status:
pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go line 239 at r2 (raw file):
Previously, bananabrick (Arjun Nair) wrote…
Does each connection have a unique
Stream?
Yes.
|
👎 Rejected by code reviews |
|
bors r+ |
|
Build succeeded: |
Fixes #104837.
Fixes #105762.
Release note: None