Repository navigation
[raft] Implement the membership of Raft groups in the Raft stores - #1902
Conversation
46a600f to
e021ddf
Compare
e13c87f to
719b757
Compare
MariemBaccari
changed the title
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
I'm not excited about introducing yet another interface but I've tried thinking about an alternative but could not come with something better.
FWIW it looks like (if Gemini did not hallucinate) etcd does deterministically generate the ID out of the peer URL and the current timestamp. But then the waiting node still has to fetch from a /members endpoint its own ID, because it does not know the timestamp. So even though they do it in reverse, they still have to expose some additional interface.
Otherwise LGTM modulo comments.
| defaultMaxSizePerMsg = 1024 * 1024 | ||
| defaultMaxInflightMsgs = 4096 / 8 | ||
|
|
||
| defaultPeerRequestTimeout = 5 * time.Second |
There was a problem hiding this comment.
How was this chosen?
And do we really want to implement this specific timeout? From a user POV this will always be embedded into another operation. I'd expect having control over the timeout of e.g. the whole 'add a node' rather than the specific request to the peer.
There was a problem hiding this comment.
That's a good point, I removed the parameter.
| logger *zap.Logger | ||
|
|
||
| nodeID uint64 | ||
| url string |
There was a problem hiding this comment.
IMO clearer:
| url string | |
| publicURL string |
| @@ -0,0 +1,30 @@ | |||
| package consensus | |||
There was a problem hiding this comment.
Yes, I wanted to keep the PRs separate. It will be deduplicated during merging / fixing conflicts.
There was a problem hiding this comment.
Consider putting into their own struct (with peerClient + creation of handler). That would help clarify in terms of abstractions that is responsible of this additional HTTP interface.
| return Node{}, stacktrace.NewErrorWithCode(dsserr.BadRequest, "invalid URL %q: %v", peerURL, err) | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, peerURL+nodePath, nil) |
There was a problem hiding this comment.
This may cause issues with extra or missing /: check url.JoinPath.
| type NodeState string | ||
|
|
||
| const ( | ||
| NodeWaiting NodeState = "waiting" |
There was a problem hiding this comment.
Please be more specific on what the node is waiting for.
| } | ||
|
|
||
| // state returns the state of this node, which is a member of its group from its first start or was removed from it. | ||
| func (c *Consensus) state() NodeState { |
There was a problem hiding this comment.
Where is the 'waiting' state? If not in implementation I'd expect a comment mentioning why it cannot be in waiting here.
There was a problem hiding this comment.
The node can't be waiting at this point since we still form the groups with --raft_peers. The waiting state comes with #1908.
1a7acf9 to
db5f7c8
Compare
db5f7c8 to
0ef8942
Compare
This PR partially addresses #1719. It makes the raftstore implement the membership management. To achieve that, the store now embeds the consensus instance to avoid having redundant wrappers.
Adding a node only takes its URL, so the ID generated by the node has to be read from the node itself using
GET /raft/nodewhich is served on the Raft listener.