[raft/aux] Add aux Raftstore - #1528
Conversation
235e51e to
e9ece61
Compare
9566c32 to
5d13119
Compare
8f85bb5 to
9e8af65
Compare
86221f4 to
3739510
Compare
3338aeb to
5e0e0cf
Compare
76cf2b8 to
f758781
Compare
8ce0e32 to
86d5b1a
Compare
34be101 to
7d3473a
Compare
f4c3ffb to
447bc05
Compare
5bcd009 to
88769b3
Compare
mickmis
left a comment
There was a problem hiding this comment.
Overall LGTM, but several minor comments
| if locality == "" { | ||
| return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Locality not set") | ||
| } | ||
| if publicEndpoint == "" { | ||
| return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Public endpoint not set") | ||
| } |
There was a problem hiding this comment.
Deduplicate this validation (push it in the caller, or factor away in a separate function, or add a validation middleware, or else) (also from memstore, and also for other methods in here).
| return stacktrace.Propagate(err, "failed to marshal payload") | ||
| } | ||
|
|
||
| _, err = r.consensus.HandleClientRequest(ctx, string(saveOwnMetadata), buf, false) |
There was a problem hiding this comment.
(not for this PR) shouldn't requestType be a type exposed alongside HandleClientRequest?
| func (r *repo) Apply(_ context.Context, _ consensus.Proposal) (any, error) { | ||
| return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "not implemented yet") | ||
| func (r *repo) Apply(ctx context.Context, proposal consensus.Proposal) (any, error) { | ||
| memRepo, err := r.memStore.Interact(ctx) |
There was a problem hiding this comment.
nit: calling Interact here looks like just an unnecessary layer of indirection, since it just returns GetRepo: call GetRepo() directly?
There was a problem hiding this comment.
Actually that is making me wonder: do we actually need the memstore abstraction? Don't we only need the memrepo? AKAIU the memstore / memrepo will exclusively be invoked by the raftstore, and never directly by the API layer.
cc @the-glu
There was a problem hiding this comment.
I don't think it cost much to keep the abstraction and to keep everything with the same "layering", so that was on purpose yes.
We probably want at some point make the memstore testable directly, or test the raftstore with others kinds of store.
| return p, nil | ||
| } | ||
|
|
||
| func GetClusterID() uint64 { |
There was a problem hiding this comment.
Why not calling GetConnectParameters instead?
| Locality string | ||
| PublicEndpoint string |
There was a problem hiding this comment.
Out of clarity, please put the json tags on the fields. This makes it explicit this struct is used for that purpose, and also makes it not necessary to expose them.
There was a problem hiding this comment.
Remarks in case of that change it, but we may want to use something else than JSON for future optimizations there.
| if err != nil { | ||
| return nil, stacktrace.Propagate(err, "failed to propose %s", getDSSMetadata) | ||
| } | ||
| if result == nil { |
There was a problem hiding this comment.
Is this necessary? The nil returned from HandleClientRequest is actually typed.
| if _, ok := result.([]*auxmodels.DSSMetadata); !ok { | ||
| return nil, stacktrace.NewError("unexpected result type: %T", result) |
There was a problem hiding this comment.
nit
| if _, ok := result.([]*auxmodels.DSSMetadata); !ok { | |
| return nil, stacktrace.NewError("unexpected result type: %T", result) | |
| if res, ok := result.([]*auxmodels.DSSMetadata); ok { | |
| return res, nil |
Chained PR: #1622 -> #1528 -> #1626 -> #1597 -> #1623
Implements the aux Raftstore.
Removes the registry as it is not needed by aux which only calls
Interact.Implements the aux repo which calls the consensus instance for each type of operation (except
GetDSSAirspaceRepresentationIDwhich is local).