roachpb: make range/replica descriptor fields non-nullable#85120
roachpb: make range/replica descriptor fields non-nullable#85120craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
871a307 to
38b8002
Compare
c91cfc4 to
7ec5794
Compare
|
I've submitted a separate DistSender fix in #85140, since there are additional issues around these replica descriptor comparisons. |
7ec5794 to
2a07859
Compare
|
|
||
| func predVoterFull(rDesc ReplicaDescriptor) bool { | ||
| switch rDesc.GetType() { | ||
| switch rDesc.Type { |
There was a problem hiding this comment.
Has making Type non-nullable eliminated this getter?
If I remember correctly, proto is converging towards GetBlah()+builders kind of APIs, so that they can make an assumption of immutability to enable optimizations. Is gogoproto different in this sense?
There was a problem hiding this comment.
Yeah, its only purpose was to convert nil to VOTER. gogoproto doesn't generate getters by default, this was a custom one. We're still stuck on the old Protobuf library and gogoproto for reasons that escape me right now.
| // l.Replica.Type, newL.Replica.Type = 0, 0 | ||
| l.Replica.Type, newL.Replica.Type = nil, nil | ||
| // replica, but that also shouldn't prevent it from extending its lease. | ||
| l.Replica.Type, newL.Replica.Type = 0, 0 |
There was a problem hiding this comment.
What do zeroes mean here? Is there a named constant for what they represent?
There was a problem hiding this comment.
The zero value is VOTER, but I think 0 better expresses the intent: we're clearing these fields because we don't care about them for comparison purposes.
| return false | ||
| } | ||
|
|
||
| switch typ := repDesc.GetType(); typ { |
There was a problem hiding this comment.
optional: Personally, I would leave this assignment to avoid retyping repDesc.Type below (here and in a few other cases in this PR), but it seems just a taste thing.
There was a problem hiding this comment.
Yeah, left it like this in a few places where we reuse the value quite a lot, but I didn't see much point in it here.
Yeah, I agree. Not sure if it's worth the effort to go back and split it up, but it would've been better to submit them separately. |
b567e0c to
bee3fbd
Compare
This patch makes all fields in range/replica descriptors non-nullable, fixing a long-standing TODO. Additionally, this fixes a set of bugs where code would use regular comparison operators (e.g. `==`) to compare replica descriptors, which with nullable pointer fields would compare the memory locations rather than the values. One practical consequence of this was that the DistSender would fail to use a new leaseholder with a non-`VOTER` type (e.g. `VOTER_INCOMING`), instead continuing to try other replicas before backing off. However, there are further issues surrounding this bug and it will be addressed separately in a way that is more amenable to backporting. The preparatory work for this was done in ea720e3. Release note: None
bee3fbd to
acdf42a
Compare
|
Spun up a mixed-version cluster with this PR and TFTR! bors r=pavelkalinnikov |
|
Build succeeded: |
This patch makes all fields in range/replica descriptors non-nullable,
fixing a long-standing TODO.
Additionally, this fixes a set of bugs where code would use regular
comparison operators (e.g.
==) to compare replica descriptors, whichwith nullable pointer fields would compare the memory locations rather
than the values. One practical consequence of this was that the
DistSender would fail to use a new leaseholder with a non-
VOTERtype(e.g.
VOTER_INCOMING), instead continuing to try other replicasbefore backing off. However, there are further issues surrounding this
bug and it will be addressed separately in a way that is more amenable
to backporting.
The preparatory work for this was done in ea720e3.
Touches #85060.
Touches #38308.
Touches #38465.
Release note: None