stateloader: untangle SynthesizeRaftState #147469
Conversation
|
This change is |
|
@arulajmani apologies if this overlaps with your split work stream too much. This untangles cluster bootstrap code from splits. I left a couple of TODOs in the code from where the splits stuff can be continued. |
Epic: none Release note: none
Epic: none Release note: none
Epic: none Release note: none
Epic: none Release note: none
Epic: none Release note: none
pav-kv
changed the title
| const replicaID = roachpb.ReplicaID(1) | ||
| // Use PreviousRelease so that we don't have to update this test on every | ||
| // version gate addition. | ||
| cv := clusterversion.ClusterVersion{Version: clusterversion.PreviousRelease.Version()} |
There was a problem hiding this comment.
TODO: find a version handle that doesn't change or get removed. Or maybe just use zero.
There was a problem hiding this comment.
Another thing you could do is s/.(*RangeVersion.*: )(.*)/\1VERSION/ on the output.
|
|
||
| eng := storage.NewDefaultInMemForTesting() | ||
| defer eng.Close() | ||
| b := eng.NewBatch() // TODO(pav-kv): make it write-only batch |
| const replicaID = roachpb.ReplicaID(1) | ||
| // Use PreviousRelease so that we don't have to update this test on every | ||
| // version gate addition. | ||
| cv := clusterversion.ClusterVersion{Version: clusterversion.PreviousRelease.Version()} |
There was a problem hiding this comment.
Another thing you could do is s/.(*RangeVersion.*: )(.*)/\1VERSION/ on the output.
|
|
||
| str, err := print.DecodeWriteBatch(b.Repr()) | ||
| require.NoError(t, err) | ||
| str = strings.ReplaceAll(str, "\n\n", "\n") |
There was a problem hiding this comment.
optional tangent: is there something we could tweak in DecodeWriteBatch or do the double newlines make sense in other situations?
| @@ -0,0 +1,10 @@ | |||
| echo | |||
| ---- | |||
| Put: 0,0 /Local/RangeID/5/u/RaftTruncatedState (0x01698d757266747400): index:10 term:5 | |||
There was a problem hiding this comment.
It makes sense for these results to be out of order given how the test is written - this lists the contents of a write batch after all - but it took me a second to realize (was wondering why u and r keys were interleaved). It is probably not worth committing the batch and then re-reading it. Maybe a comment?
| @@ -694,6 +694,8 @@ func SendEmptySnapshot( | |||
| return err | |||
| } | |||
|
|
|||
| // TODO(review): this now doesn't write RaftTruncatedState, is this fine? It's | |||
| // a snapshot, so it will not load unreplicated state. | |||
There was a problem hiding this comment.
This should be fine (better, actually). The snapshot is sent through the usual channels, so the receiver should really be managing their own raft state (via applySnapshotRaftMuLocked). TestResetQuorum exercises this btw, and I'm assuming it passes.
This PR starts untangling the opaque code in
SynthesizeHardStateandSynthesizeRaftState, with the goals of (a) making it simple, (b) decoupling log and state machine reads/writes. Right now, these funcs are used in exactly 2 cases:HardState)In both cases, the replica's state machine and log are initialized at
RaftInitialLogIndex/Term. This means that everything these funcs generate is mostly based on constants (modulo theHardStatein case 2). But it is really hard to tell by reading it. After this PR, case (1) is decoupled into its ownWriteInitialRaftStatefunc, and directly produces the right output. The "synthesize" funcs are now only used by splits, and can be further simplified in follow-up PRs.The first half of this PR adds a datadriven test for
WriteInitialRangeStateshowing the state of a typical replica created at cluster bootstrap. The printout only includesRangeID-local state, and no user space keys. The test helps ensuring that subsequent commits are no-ops.Epic: CRDB-49111