enqueue-multi compatibility fixes #1
Conversation
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
5 times, most recently
from
35d0cc2 to
6cd37a1
Compare
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
3 times, most recently
from
3feb932 to
3686b62
Compare
| - "3.0" | ||
| - "3.1" | ||
| - "3.2" | ||
| - "3.3" |
There was a problem hiding this comment.
Updated to later released ruby
| @@ -204,25 +202,36 @@ def enqueue_next_item(timestamp) | |||
| item | |||
| end | |||
|
|
|||
| def batch_delayed_items? | |||
There was a problem hiding this comment.
This starts the refactor that revives the 'non batched' mode by deciding if batching is appropriate
|
|
||
| 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? |
There was a problem hiding this comment.
log messages are updated to reflect batching
| log "queued batch of #{actual_batch_size} jobs" if actual_batch_size != -1 | ||
| else | ||
| item = enqueue_next_item(timestamp) | ||
| actual_batch_size = item.nil? ? 0 : 1 |
| require_relative 'test_helper' | ||
|
|
||
| def assert_resque_key_exists?(key) |
There was a problem hiding this comment.
better support for deprecations on redis
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
3 times, most recently
from
77e2793 to
4bdb097
Compare
| @@ -383,60 +391,115 @@ | |||
| assert_equal(1, Resque.delayed_timestamp_peek(t, 0, 3).length) | |||
| end | |||
|
|
|||
| 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.
tests to ensure both batch and non-batch work properly
| @@ -307,7 +307,7 @@ module Test | |||
| test 'redirects to overview' do | |||
| post '/delayed/cancel_now' | |||
| assert last_response.status == 302 | |||
| assert last_response.header['Location'].include? '/delayed' | |||
| assert last_response.headers['Location'].include? '/delayed' | |||
There was a problem hiding this comment.
updates to new rubies.
test/delayed_queue_test.rb
Outdated
| teardown do | ||
| Resque::Scheduler.enable_delayed_requeue_batches = batch_enabled | ||
| Resque::Scheduler.delayed_requeue_batch_size = batch_size | ||
| end |
There was a problem hiding this comment.
Non-blocking, I'm not sure I understand the need for this teardown step, more just understanding the spec. Shouldnt these be dumped between runs?
There was a problem hiding this comment.
in the setup, its touching the global singleton values for these. In these sections I'm potentially turning them on/off, so this is just to restore those values so that this section doesn't affect other tests.
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
from
2863d31 to
f63e3bc
Compare
onyxraven
force-pushed
the
enqueue-multi-rollback
branch
from
ab3225d to
ec90b99
Compare
|
Closing to recreate against upstream |
Add flag that enables/disables the batch enqueue mode for delayed jobs.
The batch enqueue mode causes issues because it starts a multi/pipeline, 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 can then lay out the compatibility risks.
References:
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 up to date with any changes.