kvserver: start making proposal generation reusable #76130
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces a new package `raftlog`. Aspirationally, this
will at some point become the de facto way of encapsulating the raft log
encoding and may play a role in programmatically constructing and
inspecting raft logs (e.g. for testing).
For now, we introduce two concepts:
- `raftlog.Entry`, which wraps a `raftpb.Entry` and all of the
information derived from it, such as the command ID, the
`kvserverpb.RaftCommand`, the configuration change (if any), etc.
- `raftlog.Iterator`, a way to iterate over the raft log in terms of
`raftlog.Entry` (as opposed to `raftpb.Entry` which requires lots of
manual processing).
Both are then applied across the codebase, concretely:
- `loqrecovery` is simplified via `raftlog.Iterator` to pull commit
triggers out of the raft log.
- debug pretty-printing is simpler thanks to use of `raftlog.Entry`.
- `decodedRaftEntry` is now structurally a `raftpb.Entry`, and again
lots manual custom unmarshaling code evaporates.
It's currently difficult to create "interesting" raft log entries if
trying to stay away from manual population of large datastructures
(which is prone to rotting), so there's zero unit testing of
`raftlog.Iterator`. However, the code is not new, instead it was
deduplicated from a few places, and is now tested through all of them;
so I don't feel to bad about it. I still think it is a priority to be
able to "comfortably" create at least "simple" raft logs, meaning we
need to be able to string together `batcheval` and entry creation at
least in a rudimentary fashion. I intend to look into this next and
add comprehensive unit tests for `raftlog.{Entry,Iterator}`.
Touches cockroachdb#75729.
Release note: None
|
This change is |
This commit introduces `TestCreateManualProposal` which attempts to carry out an "offline" (i.e. `Replica`-less) version of evaluation & entry creation (i.e. the middle and right-hand side [here]). We start with a `*roachpb.PutRequest` and end up with a payload fit for a `(raftpb.Entry).Data`. This is helpful since it provides an easily digestible "life of a proposal" read-along, but more importantly represents a step towards establishing clearer interfaces for the various stages of a request - evaluation, replication, and application - and in particular separation of these stages from the surrounding `Replica`. The test helps pinpoint pieces of code that will need to be worked on in order to reach one of the goals of cockroachdb#75729, being able to create a meaningful raft log programmatically in a unit test without also instantiating a `Replica` and, later, also applying raft logs in a similar way. As a first step, this commit extracts a method `raftCmdToPayload` from `(*Replica).propose` which translates a `kvserverpb.RaftCommand` (which is roughly the result of evaluating a `BatchRequest`) into a payload that can be handed to raft for replication. This extracted method is far from clean - it takes a few crufty parameters, lives in `kvserver` (where it is hard to import) and also uses the logging singleton which is undesirable for a library method. However, it's enough to keep the ball rolling, and can be improved on later. [here]: cockroachdb#75729 (comment) Touches cockroachdb#75729. Release note: None
sumeerbhola
reviewed
Feb 9, 2022
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)
pkg/kv/kvserver/replica_raft.go, line 401 at r2 (raw file):
// The method hands ownership of the command over to the Raft machinery. After // the method returns, all access to the command must be performed while holding // Replica.mu and Replica.raftMu.
Does calling this method require one or both of these mutexes to be held?
I see an access of r.mu.proposalBuf in the implementation. It would be nice to document this in the function comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First commit is #76126, please review there.
This commit introduces
TestCreateManualProposalwhich attempts tocarry out an "offline" (i.e.
Replica-less) version of evaluation &entry creation (i.e. the middle and right-hand side here). We start
with a
*roachpb.PutRequestand end up with a payload fit for a(raftpb.Entry).Data.This is helpful since it provides an easily digestible "life of a
proposal" read-along, but more importantly represents a step towards
establishing clearer interfaces for the various stages of a request -
evaluation, replication, and application - and in particular separation
of these stages from the surrounding
Replica. The test helps pinpointpieces of code that will need to be worked on in order to reach one of
the goals of #75729, being able to create a meaningful raft log
programmatically in a unit test without also instantiating a
Replicaand, later, also applying raft logs in a similar way.
As a first step, this commit extracts a method
raftCmdToPayloadfrom(*Replica).proposewhich translates akvserverpb.RaftCommand(whichis roughly the result of evaluating a
BatchRequest) into a payloadthat can be handed to raft for replication. This extracted method is far
from clean - it takes a few crufty parameters, lives in
kvserver(where it is hard to import) and also uses the logging singleton which
is undesirable for a library method. However, it's enough to keep the
ball rolling, and can be improved on later.
Touches #75729.
Release note: None