mma: fix a couple of todos related to changes #157930
Conversation
- subsumesChanges no longer needs the prev state as a parameter, since subsumption is only a function of the observed state and expected next state. The existing comparison with prev.IsLeaseholder was unnecessary and flawed. - Removed the todo around the panic in applyReplicaChange, since the expectation is that the range and store must exist. The callers are responsible for ensuring that. There was an old flawed idea that we would try to add the range at this point in the code, which we had long abandoned, but never removed from the code comment. Epic: CRDB-55052 Release note: None
|
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 |
wenyihu6
left a comment
wenyihu6
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @tbg)
-- commits line 7 at r1:
Apologies if this was already mentioned in comments. I need to catch up on the previous PR to get the full context. What made this check flawed, and did something change recently that allows us to remove it now? Or was this something we could have safely skipped from the start?
sumeerbhola
left a comment
sumeerbhola
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
Previously, wenyihu6 (Wenyi Hu) wrote…
Apologies if this was already mentioned in comments. I need to catch up on the previous PR to get the full context. What made this check flawed, and did something change recently that allows us to remove it now? Or was this something we could have safely skipped from the start?
Nothing changed recently to allow us to remove it. What changed is that we've properly documented what ReplicaChange represents. Specifically, the next field represents the expected final state. There was already some doubt about the comparison with prev that is in the TODO that is removed in this PR.
wenyihu6
left a comment
wenyihu6
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, sumeerbhola wrote…
Nothing changed recently to allow us to remove it. What changed is that we've properly documented what
ReplicaChangerepresents. Specifically, thenextfield represents the expected final state. There was already some doubt about the comparison withprevthat is in the TODO that is removed in this PR.
Ack.
|
bors r+ |
3 participants
subsumesChanges no longer needs the prev state as a parameter, since subsumption is only a function of the observed state and expected next state. The existing comparison with prev.IsLeaseholder was unnecessary and flawed.
Removed the todo around the panic in applyReplicaChange, since the expectation is that the range and store must exist. The callers are responsible for ensuring that. There was an old flawed idea that we would try to add the range at this point in the code, which we had long abandoned, but never removed from the code comment.
Epic: CRDB-55052
Release note: None