Skip to content

Registry server: use storage-encapsulating wrappers for all actions. #706

@timburks

Description

@timburks

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:

  1. Wrap mutating action handlers in error-translating transactional wrappers (runInTransaction).
  2. Remove all other transaction-wrapping code from the storage layer (particularly in the deletion helpers) because transactions would be provided by the new runInTransaction wrapper.
  3. Wrap non-mutating action handlers in error-translating non-transactional wrappers.
  4. Remove all internal storage calls to grpcErrorForDBError except for the calls made by the error-translating wrappers.
  5. Hide/eliminate usage of storage.NewClient (and RegistryServer.getStorageClient)

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    Status

    Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions