remove deprecated advisory lock uniqueness, consolidate insert logic #614
Conversation
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
bgentry
force-pushed
the
bg-remove-advisory-lock-unique-jobs
branch
2 times, most recently
from
9184e75 to
2b5eff2
Compare
bgentry
force-pushed
the
bg-remove-advisory-lock-unique-jobs
branch
2 times, most recently
from
95d5825 to
6264e3f
Compare
This removes the original unique jobs implementation in its entirety. It was already deprecated in the previous release. All known use cases are better supported with the new unique jobs implementation which is also dramatically faster and supports batch insertion. As part of this change, single insertions now inherit the behavior of batch insertions as far as always setting a `scheduled_at` time in the job args prior to hitting the database. This is due to the difficulty of trying to pass an array of nullable timestamps for `scheduled_at` to the database using sqlc. One side effect of this is that some tests needed to be updated because they run in a transaction, which locks in a particular `now()` time used in `JobGetAvailble` by default. Jobs inserted _after_ the start of that transaction would pick up a scheduled timestamp from Go code that is _later_ than the database transaction's timestamp, and so those jobs would never run. This was fixed in some cases by allowing `now` to be overridden by lower level callers of that query, whereas other tests were updated to simply insert jobs with a past `scheduled_at` to ensure their visibility.
bgentry
force-pushed
the
bg-remove-advisory-lock-unique-jobs
branch
from
6264e3f to
182dfb4
Compare
|
I haven't yet found the time to get the Ruby and Python clients upgraded to support this newer unique jobs path and remove the advisory lock path. However with all the stuff in flight and the much smaller user base of those libraries, I don't think I can justify holding up these other improvements any longer. I'm going to go ahead and ship this and #627 in order to move forward on #584 and the other stuff that's blocked on them. |
tigrato
pushed a commit
to gravitational/river
that referenced
this pull request
Dec 18, 2024
…iverqueue#614) This removes the original unique jobs implementation in its entirety. It was already deprecated in the previous release. All known use cases are better supported with the new unique jobs implementation which is also dramatically faster and supports batch insertion. Additionally, all insert paths are now sharing the main logic of the bulk insert query. This will make it easier to develop upcoming features and ensure a more uniform behavior with fewer special cases depending on how a job is inserted. As part of this change, single insertions now inherit the behavior of batch insertions as far as always setting a `scheduled_at` time in the job args prior to hitting the database. This is due to the difficulty of trying to pass an array of nullable timestamps for `scheduled_at` to the database using sqlc. One side effect of this is that some tests needed to be updated because they run in a transaction, which locks in a particular `now()` time used in `JobGetAvailble` by default. Jobs inserted _after_ the start of that transaction would pick up a scheduled timestamp from Go code that is _later_ than the database transaction's timestamp, and so those jobs would never run. This was fixed in some cases by allowing `now` to be overridden by lower level callers of that query, whereas other tests were updated to simply insert jobs with a past `scheduled_at` to ensure their visibility.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the original unique jobs implementation in its entirety. It was already deprecated in the previous release.
🗒️ I'm not quite ready to merge this yet (I'd like to get the Ruby + Python clients to ship the new unique jobs changes first) but I still wanted to prepare it while the context is fresh. It's also needed to simplify some upcoming Pro changes.
All known use cases are better supported with the new unique jobs implementation which is also dramatically faster and supports batch insertion.
As part of this change, single insertions now inherit the behavior of batch insertions as far as always setting a
scheduled_attime in the job args prior to hitting the database. This is due to the difficulty of trying to pass an array of nullable timestamps forscheduled_atto the database using sqlc. One side effect of this is that some tests needed to be updated because they run in a transaction, which locks in a particularnow()time used inJobGetAvailbleby default. Jobs inserted after the start of that transaction would pick up a scheduled timestamp from Go code that is later than the database transaction's timestamp, and so those jobs would never run. This was fixed in some cases by allowingnowto be overridden by lower level callers of that query, whereas other tests were updated to simply insert jobs with a pastscheduled_atto ensure their visibility.TODO
Currently based on #613.