[raft/store] Update Transact signature#1577
Conversation
MariemBaccari
force-pushed
the
change_transact_signature_prototype
branch
3 times, most recently
from
adc2bed to
558c48b
Compare
barroco
added
the
dss-raft
MariemBaccari
force-pushed
the
change_transact_signature_prototype
branch
4 times, most recently
from
5dcae06 to
373d538
Compare
MariemBaccari
force-pushed
the
change_transact_signature_prototype
branch
from
373d538 to
1c46a39
Compare
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
IMO the newly added abstractions have room for improvement. They expose concepts that look too specific to an implementation. As suggested in a comment, consider pushing raft-specific logic into the raft code.
| type Action[R any] interface { | ||
| ActionMetadata | ||
| Execute(ctx context.Context, r R) (any, error) | ||
| Payload() any | ||
| } | ||
|
|
||
| type ActionMetadata interface { | ||
| RequestType() string | ||
| IsReadOnly() bool | ||
| } |
There was a problem hiding this comment.
- functions on those interface need to be documented correctly
- it is unclear from this PR why we need a separate
ActionMetadatainterface
| return res, nil | ||
| } | ||
|
|
||
| type ActionAdapter[R any, T ActionMetadata] struct { |
There was a problem hiding this comment.
What is this? It's not used anywhere, not described in PR description, and not documented. Remove it if not necessary?
| Transact(ctx context.Context, f func(context.Context, R) error) error | ||
| // Attempt to apply the operations in action to the R Repo it is supplied. All operations performed | ||
| // on the R Repo by action will be applied or rejected atomically. | ||
| Transact(ctx context.Context, action Action[R]) (any, error) |
There was a problem hiding this comment.
Why does this get a new return parameter for the result? This looks like a new feature separate from the objective of this PR.
There was a problem hiding this comment.
With the old closure-based signature, the handlers captured results through closure variables. With the change of signature, we don't have that anymore. The only way to return a result is through the return value of Transact itself. I think this change is inseparable from the parameter change as it's the consequence of it.
| Execute(ctx context.Context, r R) (any, error) | ||
| Payload() any |
There was a problem hiding this comment.
That's a lot of any types. We'd like to avoid that in general, especially here both types are not unknown IIUC. This could indicate shaky abstractions.
| ) | ||
|
|
||
| // Action represents a set of operations to be performed on a Repo type R. | ||
| type Action[R any] interface { |
There was a problem hiding this comment.
Naming: why not Transaction? This is what it is after all, a (store) transaction on a repository R.
|
|
||
| type ActionMetadata interface { | ||
| RequestType() string | ||
| IsReadOnly() bool |
There was a problem hiding this comment.
Checking the other PR (#1580): I think I get why this is introduced here, but I think the abstractions can be improved. To me it looks like the payload, the request type, and the readonly flags are all functions of the DSS operation (i.e. the HTTP endpoint called). Given that, what about storing alongside the transaction function only the operation, and inferring all of the raft-specific details in the raft code?
That would also eliminate the additions to the generated code in #1580.
There was a problem hiding this comment.
I just pushed another proposal that aims to implement your suggestion. We could pass Transact the restapi request directly and have a Dispatch function that maps restapi requests to their corresponding execution functions. You can refer to #1525 and #1582 to have an idea of the implications of this on both the raftstore and sqlstore implementations. #1582 implements the aux store using this design.
On the one hand, we eliminate the generated methods and the Action structs and interfaces, simplifying my initial proposal but on the other hand we lose the typing at the Transact signature...
What do you think ?
| f func(context.Context, R) error | ||
| } | ||
|
|
||
| func NewActionFunction[R any](f func(context.Context, R) error) *ActionFunction[R] { |
There was a problem hiding this comment.
Good idea for the transition 👍
MariemBaccari
force-pushed
the
change_transact_signature_prototype
branch
from
7df05ef to
d700014
Compare
3 participants
Transactcurrently takes a rawfunc(ctx, R) errorclosure which works for thesqlstorebut can't be replicated by the Raftstore since it needs a request type, payload and an independently accessible function to execute on the apply side.This PR changes the interface to accept a
store.Action[R]instead in preparation for the raftstore implementation.The interface function
Executeshould contain the current action (business logic) defined in the handlers.To keep changes minimal in this PR, the
FuncAction[R]adapter wraps the existing closures as a stubAction.Follow-up PRs will remove
FuncAction[R]and extract the actions for all handlers.The PR also adds
TransactWithResult[R, Res], a generic function wrappingTransactwhich serves as a helper for result type asserts to avoid code duplication on the handler side. The reason why we need a wrapper instead of makingTransactgeneric is that Go does not currently allow generic methods.