enqueue-multi compatibility fixes #1

onyxraven · 2024-08-19T17:00:53Z

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-in, with a flag. The documentation can then lay out the compatibility risks.

References:

Heads up: resque-scheduler 4.9+ on redis-rb 4.x inadvertently breaks loner locks lantins/resque-lock-timeout#36
Adding batching to re-queuing for timestamp resque/resque-scheduler#767
Redis 5 API for multi/pipelined commands changes transaction behavior in resque-scheduler 4.9+ resque/resque-scheduler#787
NoMethodError when accessing Redis in enqueue hooks with redis 4.x resque/resque-scheduler#779
ERR MULTI calls can not be nested in release 4.9.0 resque/resque-scheduler#773

This PR also updates compatibility with ruby 3.x, and associated dependencies, lints, deprecations, etc. I realize this creates a bigger PR with a few 'style' type changes, but it helped me immensely when working to resolve this.

Compatibility integration tests with other gems (just resque-lock-timeout for now) exist in this PR - this helps prove out the fix. I hope to keep this up to date with any changes.

onyxraven · 2024-10-15T15:54:19Z

.github/workflows/rubocop.yml

+          - "3.0"
+          - "3.1"
+          - "3.2"
+          - "3.3"


Updated to later released ruby

onyxraven · 2024-10-15T15:55:19Z

.github/workflows/ruby.yml

-          - "~> 5.x"
+          - "~> 3.3"
+          - "~> 4.8"
+          - "~> 5.2"


may update the matrix again, but trying to ensure this is relatively complete, with ruby 3.0+ supporting versions and in-use redis versions. will need to look at the gemspec constraints too

onyxraven · 2024-10-15T15:55:49Z

.rubocop.yml

 AllCops:
+  TargetRubyVersion: 3.0


simplified/resolved rubocop due to updates, and supporting ruby 3.

I'm happy to remove the todo (since it covers stuff not in the below Include)

onyxraven · 2024-10-15T15:56:27Z

Rakefile

@@ -1,4 +1,3 @@
-# vim:fileencoding=utf-8


this was in every file and is now a default cop, so cleaned up. IIRC its not necessary.

onyxraven · 2024-10-15T15:57:14Z

lib/resque/scheduler.rb

@@ -204,25 +202,36 @@ def enqueue_next_item(timestamp)
        item
      end

+      def batch_delayed_items?


This starts the refactor that revives the 'non batched' mode by deciding if batching is appropriate

onyxraven · 2024-10-15T15:58:02Z

lib/resque/scheduler.rb

+                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


non batched version

onyxraven · 2024-10-15T15:58:22Z

lib/resque/scheduler/configuration.rb

      def delayed_requeue_batch_size
        @delayed_requeue_batch_size ||= \
          ENV['DELAYED_REQUEUE_BATCH_SIZE'].to_i if environment['DELAYED_REQUEUE_BATCH_SIZE']
        @delayed_requeue_batch_size ||= 100
      end

+      attr_writer :enable_delayed_requeue_batches
+
+      def enable_delayed_requeue_batches


config to enable batches (off by default)

I'm open to reversing this - and making on by default to coincide with the latest behavior of the gem.

onyxraven · 2024-10-15T15:58:38Z

resque-scheduler.gemspec

@@ -1,27 +1,26 @@
-# vim:fileencoding=utf-8
-lib = File.expand_path('../lib', __FILE__)
+lib = File.expand_path('lib', __dir__)


rubocop updates

onyxraven · 2024-10-15T15:59:02Z

test/delayed_queue_test.rb

 require_relative 'test_helper'

+def assert_resque_key_exists?(key)


better support for deprecations on redis

onyxraven · 2024-10-15T15:59:36Z

test/delayed_queue_test.rb

@@ -383,60 +390,140 @@
    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


test for batch v non batch

onyxraven · 2024-10-16T21:14:07Z

Gemfile

+gem 'racc'
+gem 'rubocop', '~> 1.4'
+gem 'rubocop-minitest'
+gem 'rubocop-rake'


moved development dependencies into the gemfile

onyxraven · 2024-10-16T21:15:09Z

lib/resque/scheduler/configuration.rb

      def delayed_requeue_batch_size
        @delayed_requeue_batch_size ||= \
          ENV['DELAYED_REQUEUE_BATCH_SIZE'].to_i if environment['DELAYED_REQUEUE_BATCH_SIZE']
        @delayed_requeue_batch_size ||= 100
      end

+      attr_writer :enable_delayed_requeue_batches
+
+      def enable_delayed_requeue_batches


I'm open to reversing this - and making on by default to coincide with the latest behavior of the gem.

onyxraven · 2024-10-16T21:23:08Z

test/delayed_queue_test.rb

@@ -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


tests to ensure both batch and non-batch work properly

onyxraven · 2024-10-16T21:23:24Z

test/resque-web_test.rb

@@ -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'


updates to new rubies.

lebeerman · 2024-10-16T23:17:22Z

test/delayed_queue_test.rb

+    teardown do
+      Resque::Scheduler.enable_delayed_requeue_batches = batch_enabled
+      Resque::Scheduler.delayed_requeue_batch_size = batch_size
+    end


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?

lebeerman

I can understand both sides re: default behavior. Opt-in for batching seems ok to me.

physik932

I don't know a ton but love the thoroughness and updates, and the separate testing repo with matrix compatibility for different versions ✨

onyxraven force-pushed the enqueue-multi-rollback branch 2 times, most recently from 354d46d to 4337268 Compare August 19, 2024 17:14

refresh ci

0a8be8e

onyxraven force-pushed the enqueue-multi-rollback branch from 4337268 to 0a8be8e Compare August 19, 2024 17:14

narrower, fix some compat issues

d7452f7

onyxraven force-pushed the enqueue-multi-rollback branch from 66fcdd2 to d7452f7 Compare August 19, 2024 18:42

onyxraven added 2 commits August 19, 2024 15:20

add flag for batch enqueue delayed

45ce5fe

wider test set

6cd37a1

onyxraven force-pushed the enqueue-multi-rollback branch from 35d0cc2 to 6cd37a1 Compare August 19, 2024 22:31

onyxraven force-pushed the enqueue-multi-rollback branch 3 times, most recently from 3feb932 to 3686b62 Compare October 15, 2024 15:51

onyxraven commented Oct 15, 2024

View reviewed changes

onyxraven force-pushed the enqueue-multi-rollback branch from aca4153 to 88236f5 Compare October 16, 2024 19:19

onyxraven added 3 commits October 16, 2024 12:52

add version tag for our branch

3e01695

ugh, fix batch flag

e071e57

rubocop to latest

ceded2c

onyxraven force-pushed the enqueue-multi-rollback branch from ccd84b1 to 77e2793 Compare October 16, 2024 19:56

rubocop corrections

4bdb097

onyxraven force-pushed the enqueue-multi-rollback branch from 77e2793 to 4bdb097 Compare October 16, 2024 20:05

onyxraven commented Oct 16, 2024

View reviewed changes

onyxraven mentioned this pull request Oct 16, 2024

Heads up: resque-scheduler 4.9+ on redis-rb 4.x inadvertently breaks loner locks lantins/resque-lock-timeout#36

Open

lebeerman reviewed Oct 16, 2024

View reviewed changes

lebeerman approved these changes Oct 16, 2024

View reviewed changes

physik932 approved these changes Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enqueue-multi compatibility fixes #1

enqueue-multi compatibility fixes #1

onyxraven commented Aug 19, 2024 •

edited

Loading

onyxraven Oct 15, 2024 •

edited

Loading

onyxraven Oct 15, 2024 •

edited

Loading

onyxraven Oct 15, 2024 •

edited

Loading

onyxraven Oct 15, 2024

onyxraven Oct 15, 2024

onyxraven Oct 15, 2024

onyxraven Oct 15, 2024

onyxraven Oct 16, 2024

onyxraven Oct 15, 2024

onyxraven Oct 15, 2024

onyxraven Oct 15, 2024

onyxraven Oct 16, 2024

onyxraven Oct 16, 2024

onyxraven Oct 16, 2024

onyxraven Oct 16, 2024

lebeerman Oct 16, 2024

lebeerman left a comment

physik932 left a comment

		require_relative 'test_helper'

		def assert_resque_key_exists?(key)

enqueue-multi compatibility fixes #1

Are you sure you want to change the base?

enqueue-multi compatibility fixes #1

Conversation

onyxraven commented Aug 19, 2024 • edited Loading

onyxraven Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

onyxraven Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

onyxraven Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebeerman left a comment

Choose a reason for hiding this comment

physik932 left a comment

Choose a reason for hiding this comment

onyxraven commented Aug 19, 2024 •

edited

Loading

onyxraven Oct 15, 2024 •

edited

Loading

onyxraven Oct 15, 2024 •

edited

Loading

onyxraven Oct 15, 2024 •

edited

Loading