raft: introduce non-protobuf structs #131511
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 |
<what was there before: Previously, ...> <why it needed to change: This was inadequate because ...> <what you did about it: To address this, this patch ...>
```
names=$(grep '^type' raft/rafttype/types.go | awk '{print $2}')
for x in $(grep -l -r raftpb raft kv raftpb); do
echo $x
sed -i '' 's/pb "github.com\/cockroachdb\/cockroach\/pkg\/raft\/raftpb"/rt "github\.com\/cockroachdb\/cockroach\/pkg\/raft\/rafttype"/' $x
sed -i '' 's/"github.com\/cockroachdb\/cockroach\/pkg\/raft\/raftpb"/"github\.com\/cockroachdb\/cockroach\/pkg\/raft\/rafttype"/' $x
for name in $names Conf Msg; do
sed -i '' "s/raftpb.$name/rafttype.$name/g" $x
sed -i '' "s/pb.$name/rt.$name/g" $x
done
done
```
andrewbaptist
force-pushed
the
2024-09-27-remove-protobuf-in-raft
branch
from
2a99d6d
to
c29c32a
Compare
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Looks interesting. I don't know if we would want rafttype.Message to be a "union" of all the message types in the end, but seems like some starting point.
It would be nice to explore having a strong type for each message instead of a "union". Then, instead of Step(Message), do something like: StepMsgApp(MsgApp), StepMsgAppResp(MsgAppResp) - these would give the caller some error or introspection specific to this step rather than a generic error. This introspection would be useful to the upper layer, e.g. RACv2 would benefit knowing which range of entries got appended (an append can be partial).
There are some performance concerns here: conversions need to copy all the fields, and allocate in some cases (e.g. when there are Entries in the message). The copies aren't too bad, but the allocations can probably be worked around in some way.
The scope of this change can probably be reduced for the moment. What are we trying to achieve? If we are trying to annotate pb.Entry with in-memory information (trace context, etc), then we could probably do this with a smaller API change, or outside raft.
| result := make([]Entry, len(entries)) | ||
| for i, entry := range entries { | ||
| result[i] = Entry{ | ||
| Term: entry.Term, | ||
| Index: entry.Index, | ||
| Type: EntryType(entry.Type), | ||
| Data: entry.Data, | ||
| } | ||
| } |
There was a problem hiding this comment.
This causes an extra allocation on every message containing entries (MsgProp, MsgApp, etc)? Also, the convertSnapshot.
We would probably want to make these messages iterable or zero-copy somehow, so that they could be stepped into raft without an extra allocation.
<what was there before: Previously, ...>
<why it needed to change: This was inadequate because ...>
<what you did about it: To address this, this patch ...>