Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handling of at in push_bulk #4601

Closed
BuonOmo opened this issue Jun 17, 2020 · 2 comments · Fixed by #4603
Closed

Better handling of at in push_bulk #4601

BuonOmo opened this issue Jun 17, 2020 · 2 comments · Fixed by #4603

Comments

@BuonOmo
Copy link
Contributor

BuonOmo commented Jun 17, 2020

Ruby version: X
Sidekiq version: 6.X

The current (>= 6) version of Sidekiq::Client.push_bulk accepts an at that could be an array of numeric timestamps. However, if the size of that array doesn't match the number of jobs, this will throw a redis error:

    # test/test_client.rb
    it 'cannot handle at with different size' do
      at = Time.new(2020, 1, 1).to_f
      first_jid, second_jid = Sidekiq::Client.push_bulk('class' => QueuedWorker, 'args' => [[1], [2]], 'at' => [at]) # will raise +Redis::CommandError: ERR value is not a valid float+
    end

I'd be glad to fix this one, but there are many way to do so... Here are the solutions I had in mind:

  1. raise properly an ArgumentError if size mismatch
  2. use the last at item as default when at is smaller:
    Sidekiq::Client.push_bulk('class' => K, 'args' => [[1], [2]], 'at' => [at])
    # would be like
    Sidekiq::Client.push_bulk('class' => K, 'args' => [[1], [2]], 'at' => [at, at])
@mperham
Copy link
Collaborator

mperham commented Jun 17, 2020

1 would be my choice.

BuonOmo added a commit to BuonOmo/sidekiq that referenced this issue Jun 18, 2020
In `Sidekiq::Client.bulk_push`, now
'at' size must be the same size as
'args' (or just be a single numeric).

Fixes sidekiq#4601
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Jun 18, 2020

Here it is then 🙂

mperham pushed a commit that referenced this issue Jun 18, 2020
In `Sidekiq::Client.bulk_push`, now
'at' size must be the same size as
'args' (or just be a single numeric).

Fixes #4601
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants