use new unique jobs implementation - #38
Conversation
Sorry, something went wrong.
This one's related to #38. It turns out that while trying to mock a context manager kind of works, it will do the wrong thing in edge cases like when an exception is thrown from inside a `with` block, silently swallowing it and causing a return that's completely wrong. There may be some way to fix the mock to make it do the right thing, but instead of getting fancier with these mocks that are already awful, instead repair the problem by defining a plain class that implements context manager and just use that.
This one's related to #38. It turns out that while trying to mock a context manager kind of works, it will do the wrong thing in edge cases like when an exception is thrown from inside a `with` block, silently swallowing it and causing a return that's completely wrong. There may be some way to fix the mock to make it do the right thing, but instead of getting fancier with these mocks that are already awful, instead repair the problem by defining a plain class that implements context manager and just use that.
This one's related to #38. It turns out that while trying to mock a context manager kind of works, it will do the wrong thing in edge cases like when an exception is thrown from inside a `with` block, silently swallowing it and causing a return that's completely wrong. There may be some way to fix the mock to make it do the right thing, but instead of getting fancier with these mocks that are already awful, instead repair the problem by defining a plain class that implements context manager and just use that.
This one's related to #38. It turns out that while trying to mock a context manager kind of works, it will do the wrong thing in edge cases like when an exception is thrown from inside a `with` block, silently swallowing it and causing a return that's completely wrong. There may be some way to fix the mock to make it do the right thing, but instead of getting fancier with these mocks that are already awful, instead repair the problem by defining a plain class that implements context manager and just use that.
This took an absurdly long time to figure out, but the problem was that the context manager mock to return an executor was swallowing a thrown AssertionError in this with block: def insert_many(self, args: List[JobArgs | InsertManyParams]) -> list[InsertResult]:
with self.driver.executor() as exec:
return self._insert_many_exec(exec, args)Thereby causing insert_many to return None and then insert fail as it tried to index element [0]. I put in a fix in #39 which seems to work. Take a look at that, we can merge it, and then it should fix the tests over here. Medium term I want to strip all these mocks out. They always seem like a good idea, but every time I use them I remember why they're so painful to work with. I'm going to time out on this PR for the day since I already spent too much time on it, but if you get blocked on one of your other work streams, maybe see if you can start looking into your no. 2 and no. 3 checkboxes.
Didn't get a chance to look at this one, but it's possible it'll require an additional sql parameter like unique_states_is_null that gets based into the SQL query. |
Sorry, something went wrong.
This one's related to #38. It turns out that while trying to mock a context manager kind of works, it will do the wrong thing in edge cases like when an exception is thrown from inside a `with` block, silently swallowing it and causing a return that's completely wrong. There may be some way to fix the mock to make it do the right thing, but instead of getting fancier with these mocks that are already awful, instead repair the problem by defining a plain class that implements context manager and just use that.
This moves the library to use the new unique jobs implementation from riverqueue/river#590 and migrates the sqlalchemy driver to use a unified insertion path, allowing bulk inserts to use unique jobs.
|
@bgentry What's the next step with respect to this one? (i.e. Do you have what you need to keep going?) Just in light of Eric checking back in today, it probably makes sense to try and make sure there's no known bugs left in this package. |
Sorry, something went wrong.
brandur
left a comment
There was a problem hiding this comment.
Damn it, I wrote this days ago and forgot to complete the review.
Sorry, something went wrong.
| args and queues. If either args or queue is changed on a new job, it's | ||
| allowed to be inserted as a new job. | ||
|
|
||
| TODO update description ⚠ ⚠️ ⚠ |
There was a problem hiding this comment.
TODO here.
Sorry, something went wrong.
| } | ||
|
|
||
| # Serialize with sorted keys and append to unique key: | ||
| sorted_args = json.dumps(args_to_include, sort_keys=True) |
There was a problem hiding this comment.
Something that just came to mind: are the JSON encodings between Go, Ruby, and Python identical on this front? Do they omit unnecessary spaces, trailing newlines, etc? Unique conflicts won't be detected properly if not.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, yeah that might be a problem. It looks like Ruby has the same behavior as Go with minimal spacing between keys:
> JSON.dump({'z': 'last', 'a': 1}.sort.to_h)
=> "{\"a\":1,\"z\":\"last\"}"
But Python puts spacing in by default:
>>> json.dumps({'z': 'last', 'a': 1}, sort_keys=True)
'{"a": 1, "z": "last"}'
However, they've got a really convenient separators keyword to dumps that takes the whitespace out and makes it like Ruby/Go:
>>> json.dumps({'z': 'last', 'a': 1}, sort_keys=True, separators=(',', ':'))
'{"a":1,"z":"last"}'
Would be worth adding that in here for sure!
Sorry, something went wrong.
There was a problem hiding this comment.
Added, thanks for digging in to confirm!
Sorry, something went wrong.
brandur
left a comment
There was a problem hiding this comment.
Modulo one comment on use of separators from yesterday, 
.
Sorry, something went wrong.
This moves the library to use the new unique jobs implementation from riverqueue/river#590 and migrates the sqlalchemy driver to use a unified insertion path, allowing bulk inserts to use unique jobs.
It's intended to be analogous to riverqueue/riverqueue-ruby#32.
Outstanding issues: