Add internal "pilot" concept #627
Conversation
bgentry
force-pushed
the
bg-proof-out-pilot
branch
from
8a285d4 to
daac06a
Compare
bgentry
force-pushed
the
bg-remove-advisory-lock-unique-jobs
branch
2 times, most recently
from
95d5825 to
6264e3f
Compare
bgentry
force-pushed
the
bg-proof-out-pilot
branch
from
daac06a to
0fd29f5
Compare
bgentry
force-pushed
the
bg-remove-advisory-lock-unique-jobs
branch
from
6264e3f to
182dfb4
Compare
This builds out a new higher level layer to sit between the Client and Driver, focused more on higher level operations than individual database queries. This allows those operations to be implemented with a combination of multiple database queries in a transaction, rather than just a single one. Naming here was tough, I really didn't like any of the options I came up with and ultimately landed on "pilot", but I'm pretty indifferent to it if a truly good one can be found. For now, the only operations implemented in the pilot are `InsertMany` and `JobSetStateIfRunningMany` (from the completer). I also made some changes within River to use these at the correct places and to propagate the pilot as needed. `JobCompleteTx` was updated to pull a `Client` out of the context in order to access this and use the same underlying query (via the pilot) as the `BatchCompleter`.
bgentry
force-pushed
the
bg-proof-out-pilot
branch
from
0fd29f5 to
13ab563
Compare
| } | ||
| defer tx.Rollback(ctx) | ||
|
|
||
| rows, err := c.pilot.JobSetStateIfRunningMany(ctx, tx, batchParams) |
There was a problem hiding this comment.
The choice here was to either wrap this with a transaction before calling into the pilot method, or to do so within it. I opted for the former primarily because it seemed to simplify the interfaces related to the driver system and avoid having to make the pilot type generic with TTx.
Open to changing any of this of course as long as it lets us do what we need to do!
| type partialExecutorMock struct { | ||
| riverdriver.Executor |
There was a problem hiding this comment.
This is now a full executor, but one that allows specific methods to be overridden. A tx variant of it was also needed because of the txn change in the completer.
| func serviceName(service startstop.Service) string { | ||
| elem := reflect.TypeOf(service).Elem() | ||
| return elem.PkgPath() + "." + elem.Name() |
There was a problem hiding this comment.
I made this change to the make the internal names more unique because I had originally had two pro maintenance processes with the same name in different packages, and I lost a fair bit of time trying to figure out why one of them wasn't working. I don't think there are downsides/risks here other than just the internal name being the longer & more complicated path+type combo since it's only used internally.
| "github.com/riverqueue/river/rivertype" | ||
| ) | ||
|
|
||
| type StandardPilot struct { |
There was a problem hiding this comment.
This one is just a passthrough to the driver queries as of now.
|
I'd still call this a bit of a work in progress and I think it will evolve or even change significantly, but for now I think it's good enough to move forward and iterate on. |
|
Would you mind telling me what this is even for? The implementation is basically empty now, what's the pilot supposed to be doing? |
|
@nexovec essentially we need a way to be able to override or extend certain functions with more complex implementations to power upcoming River Pro features. Our "driver" concept mainly exists to power low level queries across different database engines with query or type differences, so abusing/reusing that concept for something like a higher level grouping of functionality didn't feel appropriate, hence the added layer. As of now the "pilot" in the OSS version is just a passthrough to the driver, I'm not sure whether that will change or not in the future, though it's unlikely to be anything users need to care about. |
|
So it's something for workflows? |
|
Not currently being used for workflows, no. But yes for the next Pro feature and some of the ones after that 🚀 |
This builds out a new higher level layer to sit between the Client and Driver, focused more on higher level operations than individual database queries. This allows those operations to be implemented with a combination of multiple database queries in a transaction, rather than just a single one. Naming here was tough, I really didn't like any of the options I came up with and ultimately landed on "pilot", but I'm pretty indifferent to it if a truly good one can be found. For now, the only operations implemented in the pilot are `InsertMany` and `JobSetStateIfRunningMany` (from the completer). I also made some changes within River to use these at the correct places and to propagate the pilot as needed. `JobCompleteTx` was updated to pull a `Client` out of the context in order to access this and use the same underlying query (via the pilot) as the `BatchCompleter`.
This builds out a new layer to sit between the Client and Driver, focused more on higher level operations than individual database queries. This allows those operations to be implemented with a combination of multiple database queries in a transaction, rather than just a single one.
Naming here was tough, I really didn't like any of the options I came up with and ultimately landed on "pilot", but I'm pretty indifferent to it if a truly good one can be found. I spent enough time trying to find a good one that I truly don't care at this point 😆
For now, the only operations implemented in the pilot are
InsertManyandJobSetStateIfRunningMany(from the completer). I also made some changes within River to use these at the correct places and to propagate the pilot as needed.JobCompleteTxwas updated to pull aClientout of the context in order to access this and use the same underlying query (via the pilot) as theBatchCompleter.Based on #614 (including #624).