Delayed enqueue batching opt-out #796
Conversation
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
from
ca3cfd5 to
5c390c6
Compare
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
2 times, most recently
from
1a48457 to
f4243df
Compare
| def assert_resque_key_exists?(key) | ||
| if Gem::Requirement.create('< 4').satisfied_by?(Gem::Version.create(Redis::VERSION)) | ||
| assert(!Resque.redis.exists(key)) | ||
| else | ||
| assert(!Resque.redis.exists?(key)) | ||
| end | ||
| end |
There was a problem hiding this comment.
Nice. This was around the signature change for exists?, right? https://github.com/redis/redis-rb/blob/master/CHANGELOG.md#420
There was a problem hiding this comment.
Yep thats correct. This was throwing the deprecation warning without this adapter
|
@onyxraven just a heads up, mixing in big unrelated changes to the feature make this tough to approve/merge. Can we break apart the ruby compatibility component from the actual feature here? Dropping support for previous versions of ruby is usually a maintainer decision and since it's bundled together here, it makes the rest of the PR unmergeable at this time. |
Yep you're right. I can definitely split those up. This started out as a private-only fork and I forgot I had pulled all my changes over. I'll update this branch/PR to reflect just the batch opt-out change. |
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
3 times, most recently
from
4fe6f23 to
291fbc2
Compare
| - "3.5" | ||
| - "3.6" | ||
| redis-version: | ||
| - "~> 3.x" |
There was a problem hiding this comment.
I wanted to add this older gem version so that it validated that both work (especially with regard to the batches)
| log "Processing delayed items for timestamp #{timestamp}, in batches of #{batch_size}" | ||
| message = "Processing delayed items for timestamp #{timestamp}" | ||
| message += ", in batches of #{batch_size}" if batch_delayed_items? | ||
| log message |
There was a problem hiding this comment.
I tried to keep the log messages consistent for batch vs non-batch
|
|
||
| test 'enqueue_delayed_items_for_timestamp enqueues jobs in 2 batches' do | ||
| t = Time.now + 60 | ||
| context 'non-batch delayed item queue' do |
There was a problem hiding this comment.
new tests for disabling batch
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
from
291fbc2 to
34e6cf7
Compare
@PatrickTulskie I've done that. The changeset only has the direct changes now. |
3 participants
This PR adds a flag that enables/disables the batch enqueue mode for delayed jobs.
The batch enqueue starts a multi/pipeline around the job enqueue for delayed jobs when they are being executed, but this breaks a number of other plugins that rely on doing Redis work within that enqueue window.
The current approach is to make the batch behavior opt-out, with a flag. The documentation is updatd to lay out the compatibility risks.
References:
lantins/resque-lock-timeout#36
#767
#787
#779
#773
This PR also updates compatibility with ruby 3.x. This creates a slightly larger PR, but was required for my use cases and helps to move the gem forward. Some projects choose to make dropping support for old rubies a major version bump, but I'm undecided if thats necessary, and would defer to project maintainers.
Compatibility integration tests with other gems (just resque-lock-timeout for now) exist in this repo - this helps prove out the fix. I hope to keep this other repo up to date with any changes.