Use new unique jobs implementation #32
Conversation
| data_set = data_set.where(queue: get_params.queue) if get_params.queue | ||
| data_set = data_set.where(state: get_params.state) if get_params.state | ||
| data_set.first ? to_job_row(data_set.first) : nil | ||
| end |
There was a problem hiding this comment.
Definitely nice that this can all go away.
There was a problem hiding this comment.
Thanks for sending this over! Looking good so far. Does result in some nice clean up in a few places which is great.
Now that bulk inserts support unique jobs and can return full rows, should we adjust the return signature of insert_many like we did in River itself? This would allow detection of duplicates even with bulk inserts.
Yeah, seems like a good idea to keep the API the same as what's in Go. The return values might be useful to someone too.
It seems like the client specs are only hitting the sequel driver and not ActiveRecord. I haven't actually fully converted the AR side yet anyway, and I'm not sure if this note is going to turn out to be a blocker. Likewise with whether I can make the unified insert path work well in AR.
Yeah, I need to get the tests sorted out a little better. I didn't have a fully unified theory around how testing should work, so originally I had it so that the driver specs would test everything end to end using the database, and the client tests would use a mock layer. The mock layer turned out to be so useless for testing anything and was hard to maintain though, so I ended up just getting rid of it in favor of testing end to end in the client specs as well. They arbitrarily use only one driver, but it's probably okay since the driver specs should be testing pretty thoroughly themselves (see spec/driver_shared_examples.rb).
Regarding the note: hmm, that is unfortunate. This might be require some more bushwhacking around AR source code. There's gotta be a way to make it work. With any luck, they might've added some fixes since I wrote that.
While building this I realized I may have neglected an issue in #590, which is that if you attempt to insert duplicate rows in the same query, you'll get a Postgres error about not being able to UPSERT a row from the same query. We could handle this by scanning all rows-to-be-inserted to see if there are duplicate unique params, and we may want to do that in the Go library too for better devex. Thoughts?
Yeah I could see it either way probably. It might be okay just to leave this error to bubble up to the user. I've run into it a number of times during my time working with Go/Postgres over the last four years, and although every time I was annoyed at Postgres for the generic-sounding/kind of unhelpful error, it turned out that I was doing something wrong in my Go. e.g. I wasn't going through a data set which I was about to insert and making it unique, which I should've been doing.
I removed code specific to the old path and didn't attempt to provide a migration path in this client library given that the Go library had a sequence of releases which were compatible with either strategy. If we want to do that, it gets a bit more complex, but lmk what you think.
Yeah, let's just get it fixed without a migration path. It's a little suboptimal, but this library doesn't have enough users to spend a huge amount of effort on that. If people run into problems we'll ask them to upgrade everything to latest.
bgentry
force-pushed
the
bg-updated-unique-jobs
branch
3 times, most recently
from
1fde20c to
07a2f6c
Compare
bgentry
force-pushed
the
bg-updated-unique-jobs
branch
from
07a2f6c to
561cf00
Compare
|
Alright @brandur, this should be actually ready now. ActiveRecord stuff seems to work just fine with bulk unique inserts, or at least with a unified insert path. |
bgentry
force-pushed
the
bg-updated-unique-jobs
branch
from
561cf00 to
6213718
Compare
| unless unique_opts.exclude_kind | ||
| unique_key += "&kind=#{insert_params.kind}" | ||
| end | ||
|
|
||
| if unique_opts.by_args |
There was a problem hiding this comment.
Realizing that this PR does not include the expansion of by_args to allow uniqueness by a subset of args. It also does not yet implement the required change to alphabetically sort the json by key name. The latter is required, though the former could conceivably be added later. Thoughts?
There was a problem hiding this comment.
I think I got both of these implemented pretty easily by setting by_args to be either a bool or an array of keys to be extracted from the JSON before sorting.
There was a problem hiding this comment.
Oh nice. Well, that makes it easy then.
bgentry
force-pushed
the
bg-updated-unique-jobs
branch
2 times, most recently
from
33e8627 to
66dca34
Compare
bgentry
force-pushed
the
bg-updated-unique-jobs
branch
from
66dca34 to
2831d33
Compare
Prepare release 0.8.0, mostly containing the changes in #32.
Prepare release 0.8.0, mostly containing the changes in #32.
This moves this library to use the new unique jobs implementation from riverqueue/river#590 and migrates the Sequel driver to use a unified insertion path, allowing bulk inserts to use unique jobs.
Questions:
insert_manylike we did in River itself? This would allow detection of duplicates even with bulk inserts.UPSERTa row from the same query. We could handle this by scanning all rows-to-be-inserted to see if there are duplicate unique params, and we may want to do that in the Go library too for better devex. Thoughts?