rac2: specify lock ordering, and add ReplicaSet #128706
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 |
kvoli
left a comment
kvoli
left a comment
There was a problem hiding this comment.
roachpb.ReplicaSet, I don't feel strongly enough to block on it but it'd be worthwhile writing down why we don't use it.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @pav-kv and @sumeerbhola)
pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 47 at r1 (raw file):
// // Requires replica.raftMu to be held. SetReplicasRaftMuLocked(ctx context.Context, replicas ReplicaSet) error
nit: do we need to use a new replica set type, instead of using the existing roachpb.ReplicaSet? It makes sense to use it internally, within the datastructure if we want map accesses but as an argument it might be nicer to use roachpb.ReplicaSet, which is a wrapped slice and used in many other parts of KV for similar purposes.
sumeerbhola
left a comment
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 47 at r1 (raw file):
Previously, kvoli (Austen) wrote…
nit: do we need to use a new replica set type, instead of using the existing
roachpb.ReplicaSet? It makes sense to use it internally, within the datastructure if we want map accesses but as an argument it might be nicer to useroachpb.ReplicaSet, which is a wrapped slice and used in many other parts of KV for similar purposes.
The replicaRACv2Integration uses a map for lookups by ReplicaID. There is also one case of such lookups in RangeControllerImpl. These sets are only replaced with a new map (never updated), so it is convenient to share.
And it reduces unnecessary code: in replicaRACv2Integration it would need to produce a map for its internal use and produce a roachpb.ReplicaSet for RangeController, which would then proceed to convert the roachpb.ReplicaSet to a map.
Informs cockroachdb#128308 Epic: CRDB-37515 Release note: None
sumeerbhola
force-pushed
the
replicaset_rac2
branch
from
7941334 to
1ddc364
Compare
sumeerbhola
left a comment
sumeerbhola
left a comment
There was a problem hiding this comment.
TFTR!
I tweaked the commentary about ReplicaSet and SetReplicasRaftMuLocked.
Reviewable status:
kvoli
left a comment
kvoli
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
bors r=kvoli |
3 participants
Informs #128308
Epic: CRDB-37515
Release note: None