Repository navigation
[raft] Define admin API in OpenAPI and generate it - #1776
MariemBaccari wants to merge 4 commits into
Conversation
08fbd92 to
6327a60
Compare
MariemBaccari
changed the title
b8073f1 to
75c6087
Compare
| required: true | ||
| description: ID of the node to remove. | ||
| schema: | ||
| type: string |
There was a problem hiding this comment.
Because node IDs are random uint64 values (will become random in some follow-up PRs) which doesn't fit in openapi's integer (max int64).
| - name: group | ||
| in: path | ||
| required: true | ||
| description: Name of the Raft group (aux, rid or scd). |
BenjaminPelletier
left a comment
BenjaminPelletier
left a comment
There was a problem hiding this comment.
I'd recommend dedicating a PR to just this file -- in my opinion, interfaces and APIs are the most important elements by far and it's worth thinking very hard about them.
I've found that structuring an API according to resources rather than RPCs proactively avoids many edge-case gotchas that don't become obvious until later. For instance, here I would perhaps consider retries due to dropped packets, especially as they might interact with leader failures. I don't think we need to follow Google's AIPs exactly, especially since they assume protobufs. But, where we decide to deviate from their advice, I think we should have clear reasoning that our use case won't suffer from the problems that lead Google to choose those design patterns. This API is already fairly RESTful (hooray), but I've noted some places I think it diverges below.
Another thing I think might be worth considering is whether we might want to make membership changes a resource collection. So requesting a membership change (addition, deletion, anything else like changing from follower to voter) would be POST /v1/raft/groups/scd/membership_changes which I would expect to create a pending membership change which would then progress through its various stages (or initially it could simply be synchronous and block until complete).
| openapi: 3.0.2 | ||
| info: | ||
| title: DSS Admin API | ||
| version: 1.0.0 |
There was a problem hiding this comment.
Let's keep this at 0.x.x until the initial release of raftstore -- a 1.0.0-level API should absolutely have appropriate security controls.
| description: Human-readable message indicating what error occurred and/or why. | ||
| type: string | ||
|
|
||
| RaftMember: |
There was a problem hiding this comment.
Wouldn't a node be added as a learner first so it could spend some time catching up before being made a voting member? If so, where is the state differentiation between learner and voter?
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/RaftGroupMembers' |
There was a problem hiding this comment.
I recommend returning nothing (204): https://google.aip.dev/135
Co-authored-by: Benjamin Pelletier <BenjaminPelletier@users.noreply.github.com>
83a447b to
cb76872
Compare
|
@BenjaminPelletier Thanks a lot for the feedback. I addressed the first few batch of suggestions in my latest push. I will take a look at the Google AIPs and see how to best comply with them. |
This PR is a follow-up to #1780 and partially addresses #1719 by defining a new admin API which will handle cluster membership changes.