[Raft] Add Raft store stubs#1451
Conversation
the-glu
left a comment
the-glu
left a comment
There was a problem hiding this comment.
Thanks for the contribution, a few suggestions based on the store architecture.
| if err != nil { | ||
| return stacktrace.Propagate(err, "Failed to create remote ID server") | ||
| // Initialize aux and remote ID if implemented by the store | ||
| if params.GetStoreParameters().StoreType != params.RaftStoreType { |
There was a problem hiding this comment.
I would not have explicit references to store type here: the code in that file should not initialize aux/rid/scd server iff it didn't get a store, but it's the responsibility of the store interface behind to use the params and to return (or not) a store based on them.
There was a problem hiding this comment.
I addressed this by using a stacktrace error code to detect unsupported store types instead. Does that seem reasonable ?
There was a problem hiding this comment.
Yes thanks, just added a comment about normalization.
the-glu
left a comment
the-glu
left a comment
There was a problem hiding this comment.
LGTM, Just a small fix for scd's CodeUnsupported normalization.
| panic("not implemented") | ||
| } | ||
|
|
||
| // Interact returns a read-only repository that can be used to query the store without proposing a Raft entry. |
There was a problem hiding this comment.
Just for info: there are a few cases when interact() is used in RW mode, but that in RID&aux
There was a problem hiding this comment.
I fixed the comment, thanks
| // Initialize aux | ||
| auxV1Server, err = createAuxServer(ctx, locality, *publicEndpoint, *scdGlobalLock, logger) | ||
| if err != nil { | ||
| if stacktrace.GetCode(err) == store.CodeUnsupported { |
There was a problem hiding this comment.
No special handling should be required here. I think it is fine to let the error bubble up and return a 500 (or better, 501*) if they are invoked.
Alternatively, if we want to not register some endpoints, this should be made possible explictly through some config flags (just like the one for scd).
- IMO the tool
openapi-to-go-server should be adding auto-generated 501 responses in the generated models. Then we could wire the handlers so that adsserr.NotImplemented` maps to a HTTP 501. But that's not within the scope of this PR.
There was a problem hiding this comment.
Since rid and aux won't be implemented in the first stages of Raft store integration, removing the special handling here will prevent the core-service from starting with just scd. If we add stub rid and aux interfaces, the initialization will fail due to Interact and SaveOwnMetadata not being implemented.
On the other hand, implementing new config flags is invasive considering that rid and aux will be implemented eventually so we will have a temporary config change.
Could you specify which alternative you had in mind ? I'm not sure how to go about this.
3 participants
This PR adds Raft store scd stubs to prepare for the incoming implementation.
Since rid and aux will not initially be supported, it skips them.