-
Notifications
You must be signed in to change notification settings - Fork 93
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Push rescuer down into
internal/maintenance + JobRow to rivertype
Backstory: We've made some attempt to modularize River so that it's entire implementation doesn't sit in one top-level package. We had some success with this approach (see `internal/`), but after making some progress we eventually hit a brick wall with most of the core types, and even regressed as one of the queue maintainers (rescuer) had to live in the top-level package so it could access worker-related information. Modularity aside: I realized the other day that we also have another problem on the horizon. We have a new driver system, but current drivers like `riverpgxv5` cheat by implementing a trivial wrapper around pgx. Long term, these drives are going to have to look more like how `DBAdapter` looks today. They'll have functions like `JobInsert` and `JobComplete` specific to the particular driver implementation, and those functions will need to take and return types representing things like insert parameters and job return values. It might be possible to use types from `river` for this, but we'd have to invert the dependency requirement from `river` -> `riverpgxv5` and change a lot of tests. Being able to depend on a shared submodule (which `rivertype` could become) would be a lot easier. ## Approach I wanted to try and fix that and pursue further modularization, but it wasn't easy. I tried every trick in the book to cast types from something internal to public-facing etc., but as implemented, it's either not possible, or only possible with considerable downsides (see bottom). However, there is a change we can make that unlocks everything: `JobRow` is the core type that needs to be shared amongst everything. If it could live in a shared package, internal packages get the ability to reference it and whole puzzle unjumbles into crystal clarity. Previously, I'd convinced you that we could put core types into a separate `rivertype` package, which we did, and then later undid after finding a few techniques to share some internally referenced types. Here, we bring that idea back to some degree, but only for one type: `JobRow` (and okay, `AttemptError` too). This doesn't produce anywhere near the same degree of required API adjustments because `JobRow` is generally something that's returned from functions, and therefore `rivertype` never needs to be referenced. It's also embedded on `Job`, so its properties are available there again without importing `rivertype`. `Job` stays in the top-level package so worker `Work` signatures are not affected. Signatures of the error handler and a few other types are affected, but it's certainly the less common case. With that move in place we go on to demonstrate that further encapsulation becomes possible. We move the new work unit primitives into a shared internal package, and then make use of them in the rescuer so that it can decouple itself from the concept of a top-level worker. With that done, we can push it into `internal/maintenance` where it was supposed to be originally. Another benefit: when introducing `rivertest`, we had to duplicate the `jobRowFromInternal` function because of the same type problems mentioned above. With `JobRow` not in a shared package, we can push this helper into `dbsqlc` and have it shared amongst `river` and `rivertest`, eliminating the necessity for this extra function. And from here all the other pieces can fall too: job executor, producer, etc. could all be encapsulated separately if we'd like them to be. ## Alternatives considered ### Struct conversion by removing `AttemptError` Go allows struct to easily be converted from one to another so you could potentially have `river.JobRow(internalJobRow)`, but the limitation is that you can't do it as soon as substructs (e.g. `AttempError`) are introduced, even if the substructs are also identical to each other. One thing that we could potentially do here is change `AttemptError` back to a plain set of `[]byte`s and add an unmarshaling helper like `UnmarshalAttemptErrors() []AttemptError`. I think it'd work, but also has downsides of its own: * Prevents us from having adding other substructs to the job row in the future. * Caching unmarshaled attempt errors is difficult because if either struct picked up an unexported member, it can no longer be converted to the other. * The conversions and transform wrappers that'd need to happen to pass back and forth with interfaces like `ErrorHandler` would be tricky and add probably cruft. ### Use of unsafe Another trick you can do similar to the above: if you're sure two structs really are identical, you can convert one to the other with `unsafe.Pointer`, even if they include substructs like `AttemptError`. I made a prototype of this and it definitely works, so I really considered this as a good possible way forward. However, use of the `unsafe` package is frowned upon, and it's also banned from some hosted providers like Google App Engine, which seems bad for our use case.
- Loading branch information
Showing
22 changed files
with
450 additions
and
439 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.