-
Notifications
You must be signed in to change notification settings - Fork 34
Description
Several issues (#623, #448, #382) raise concerns about the lack of transactionality in mutating action handlers. A few different solutions are suggested, including table-level locking (to me this seems overly coarse) and modifying the storage layer so that a single storage call can be made for each action (to me this would push so much logic into the storage layer that it loses value as an abstraction). Another approach would expose the gorm transaction-handling functions to the action handlers, allowing them to explicitly start and commit or rollback transactions (with the fairly large number of mutating actions, this seems that it would excessively replicate transaction-handling code).
The gorm Transaction function provides a pattern that we can use to more directly and simply wrap transactions at the action handler level. This function exposes a simple interface that takes a function as argument - this function performs arbitrary database operations that are wrapped in a transaction, and the Transaction function appropriately begins, commits, and rolls back transactions, as appropriate. A similar function can be easily added to the storage package, accepting a callback that is passed a storage.Client and that returns an error. This function can contain most or all of the implementation of a mutating action handler, and its return value provides a convenient place to convert database errors into gRPC-compatible errors. (This is currently done with calls to grpcErrorForDBError that are scattered throughout the storage package.)
To further build on this error-conversion benefit, non-mutating action handlers could call a similar function in the storage layer that is identical to the proposed runInTransaction function but doesn't use a transaction.
If all action handlers called one of these two wrapper functions, there would be no need to call storage.NewClient, and this could be made private.
This work could be done in stages:
- Wrap mutating action handlers in error-translating transactional wrappers (
runInTransaction). - Remove all other transaction-wrapping code from the storage layer (particularly in the deletion helpers) because transactions would be provided by the new
runInTransactionwrapper. - Wrap non-mutating action handlers in error-translating non-transactional wrappers.
- Remove all internal
storagecalls togrpcErrorForDBErrorexcept for the calls made by the error-translating wrappers. - Hide/eliminate usage of
storage.NewClient(andRegistryServer.getStorageClient)
Metadata
Metadata
Assignees
Labels
Type
Projects
Status