kvserver: consolidate RaftTruncatedState handling #136536
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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 |
pav-kv
force-pushed
the
simplify-trunc-state-handling
branch
2 times, most recently
from
74b97f7 to
f00afad
Compare
There was a problem hiding this comment.
But we can't rule out that there are still raft log entries around that have the zero, right? In contrived scenarios, these could pop up for application after an upgrade to the release that contains this PR. Don't we - at least technically - need a below-raft migration here?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)
There was a problem hiding this comment.
Good point, yeah, I'll remove this part of the PR. Unless we've had any below-raft migrations after Feb 2022. We don't need a fully-fledged migration, just the wait/sync all state machines aspect of it. Any other below-raft migration would give us this for free.
Reviewable status:
RaftExpectedFirstIndex used to be zero before f9dee66 which started setting this field after a LooselyCoupledRaftLogTruncation version gate. Since then, the version gate has been removed, and the field is set unconditionally. But we still need to have done at least one below-raft migration to safely say that this field can't be zero. Added a TODO. Epic: none Release note: none
Epic: none Release note: none
Epic: none Release note: none
pav-kv
force-pushed
the
simplify-trunc-state-handling
branch
from
f00afad to
1f16c56
Compare
|
TFTR! bors r=tbg |
136536: kvserver: consolidate RaftTruncatedState handling r=tbg a=pav-kv This PR simplifies handling the `RaftTruncatedState` eval result, and adds TODO to remove a zero check after the next below-raft migration. `RaftExpectedFirstIndex` used to be zero before f9dee66 which started setting this field after a `LooselyCoupledRaftLogTruncation` version gate. Since then, the version gate has been removed, and the field is set unconditionally. But we still need to have done at least one below-raft migration to safely say that this field can't be zero. Related to #136109 Co-authored-by: Pavel Kalinnikov <[email protected]>
|
Build failed: |
|
The CI failure in bors is unrelated: #136548. |
|
bors retry |
136497: kv: use crtime.Mono in sendPartialBatch r=RaduBerinde a=RaduBerinde Informs: #133315 Release note: None 136536: kvserver: consolidate RaftTruncatedState handling r=tbg a=pav-kv This PR simplifies handling the `RaftTruncatedState` eval result, and adds TODO to remove a zero check after the next below-raft migration. `RaftExpectedFirstIndex` used to be zero before f9dee66 which started setting this field after a `LooselyCoupledRaftLogTruncation` version gate. Since then, the version gate has been removed, and the field is set unconditionally. But we still need to have done at least one below-raft migration to safely say that this field can't be zero. Related to #136109 Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Pavel Kalinnikov <[email protected]>
|
Build failed (retrying...): |
|
bors cancel |
|
Canceled. |
|
CI should be fixed by #136583. Retrying to enter the queue after it. |
|
bors r+ |
|
Build succeeded: |
This PR simplifies handling the
RaftTruncatedStateeval result, and adds TODO to remove a zero check after the next below-raft migration.RaftExpectedFirstIndexused to be zero before f9dee66 which started setting this field after aLooselyCoupledRaftLogTruncationversion gate. Since then, the version gate has been removed, and the field is set unconditionally.But we still need to have done at least one below-raft migration to safely say that this field can't be zero.
Related to #136109