Allow custom locker implementations #575
Conversation
| var fn func(context.Context) error | ||
|
|
||
| switch t := s.querier.(type) { | ||
| case *dialectquery.Postgres: |
There was a problem hiding this comment.
IMO it would not scale well - it will be a lot of switch cases.
Instead, we can just provide multiple interface implementations later.
| } | ||
|
|
||
| type PostgresLockerOptions struct { | ||
| LockID int64 |
There was a problem hiding this comment.
Thanks to that, it's possible to provide a custom LockID
| LockMode LockMode | ||
|
|
||
| // CustomLocker overrides the default locker. | ||
| CustomLocker Locker |
There was a problem hiding this comment.
IMO it could be named a bit better (just Locker may be a bit misleading - I wanted to emphasize that it's not required)
By default, locker will be chosen based on the dialect (now just Postgres is supported).
It can be overridden by any implementation - provided by goose (like Postgres), or replaced by something else (Redis or whatever).
| LockID int64 | ||
| } | ||
|
|
||
| func NewPostgresLocker(opts PostgresLockerOptions) *PostgresLocker { |
There was a problem hiding this comment.
Earlier, this locker was encapsulated (by putting it in an internal package).
But it can't be encapsulated if we want to allow injecting this provider (and configuration).
|
Figured I'd drop by and say I'd be open to supporting a custom locker implementation. If we can find a nice interface for it and make it extensible, seems reasonable. It's been very busy for me lately, but I'll slowly pick off these PR's / issues with thoughts. I usually view these and take time to think them through, coming back and dropping comments. |
There was a problem hiding this comment.
Alright, thanks for putting up a PR for discussion.
I think I like the idea of exposing the Locker interface and defining it in locker.go in the main package, and then exposing a setter like WithLock().
However, instead of defining the implementations within package goose, instead I think we can move them out to pkg/lock and provider implementations, starting with postgres.
We can then remove LockMode entirely, and user can supply a locker interface instead. Brining their own or using one of pkg/lock.
The downside of this approach is that it limits the implementation. I.e., I was hoping to add transaction-level locking pg_advisory_xact_lock, but because the locker itself is exposed this wouldn't be possible.
Maybe there's an opportunity for both SessionLocker and TransactionLocker interfaces 🤔.
|
Going to close this PR in favor of the Made sure to mention you in the announcement post, I think the idea of a custom locker is nice 👍 (hopefully it's implementation is easy enough to extend). https://pressly.github.io/goose/blog/2023/goose-provider/#database-locking |
In this PR I'm adding an option to inject custom locker (or configure already existing one).