toptal
Delayed Sidekiq strategy errors by passing invalid Array type to redis client
I am trying to import using the Delayed sidekiq strategy. However I am getting a redis client error for passing an invalid array type. From readings on other repository issues this seems to be a common case with gems integrated with redis commands which pass nested arrays to sadd call.
Expected behavior
I would expect it to kick off a redis job for sidekiq but it errors.
Actual behavior
The stacktrace throws an error from redis-client. See here someone else had come across this similar error for sidekiq-batch gem that had to be fixed to pass the correct format.
Stacktrace
TypeError: Unsupported command argument type: Array
from /Users/myuser/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/redis-client-0.22.2/lib/redis_client/command_builder.rb:39:in `block in generate'
This issue seemed to bubble up to this LUA script call here.
Steps to reproduce the problem
I setup with the standard delayed sidekiq settings
Chewy.settings = {
strategy_config: {
delayed_sidekiq: {
latency: 3,
margin: 2,
ttl: 60 * 60 * 24
}
}
}
You can reproduce by:
ProductsIndex.import!([1], strategy: :delayed_sidekiq)
Or even more specific I was diagnosing the call further down the stack to validate the inputs were correct.
Chewy::Strategy::DelayedSidekiq::Scheduler.new(ProductsIndex, [1]).postpone
This is only applicable on 8.0.0.pre.beta tag.
Version Information
Share here essential version information such as:
- Chewy version: 8.0.0.pre.beta (commit = 6a2176f7d92c4818f1a1f572aed339b39aef087e)
- Elasticsearch version: 8.5.1
- Ruby version: 3.1.2
- Rails version: 6.1.7.9
- Redis gem version: 4.2.0, 4.8.1, and 5.0.2 (latest)
I have done further debugging and found the problematic input:
On this line
The following input from the variable element is an array:
["chewy:delayed_sidekiq:ProductsIndex:1730932392", "chewy:delayed_sidekiq:ProductsIndex:timechunks"]
It's a bit tricky to debug because of the static LUA script.
If I debug one level up the keys argument with the value [timechunk_key, timechunks_key] from this line matched the same value:
["chewy:delayed_sidekiq:ProductsIndex:1730932851", "chewy:delayed_sidekiq:ProductsIndex:timechunks"]
So somewhere this keys variable is passed along to one of those redis calls in the LUA script as an array.
I'm thinking it's the sadd or zrank call but I'm not as familiar with LUA.
Also this LUA script was failing too for a similar error: https://github.com/toptal/chewy/pull/937/commits/784fe38f9a935770852d4494476f84ef019e0aa3#diff-3dd6f3f828085be7751a3490204c6604a00727b81650637c632608d22ee28138R42
Okay it came down to this PR that caused this error for delayed_sidekiq. If I revert this specific change everything is functional again. https://github.com/toptal/chewy/pull/937
cc @skcc321 @konalegi
My guess is the the LUA script is evaluating something as an array instead of a single string from one of the local variables. Something is different from LUA vs ruby in the way the inputs are transformed.
Well this is still broken, based on upvotes it seems like this is an issue for others too. @skcc321 or any others can you verify this error is reproducible? If not reproducible, can you provide which version of sidekiq you may be using? As I can't find a way to make delayed sidekiq functional with the LUA script change at all.
After further digging, I discovered the LUA script is not being formatted correctly by the Sidekiq redis client wrapper (instance of Sidekiq::RedisClientAdapter::CompatClient must behave differently and the specs mock Sidekiq redis call). It is sending nested arrays which is problematic as you can see from the link above to redis-client error.
I found this PR that showed a LUA script being called within Sidekiq. I mimicked a similar convention and the proper data structure was sent for keys and argv. Now everything works with the LUA script without issue.
For example I did something like this for both worker.rb and scheduled.rb LUA script:
members = extract_members(redis, keys: [], argv: [processor_key, score, Scheduler::KEY_PREFIX])
....
def extract_members(conn, keys: [], argv: [])
if @lua_extract_members_sha.nil?
@lua_extract_members_sha = conn.script(:load, LUA_SCRIPT)
end
conn.call("EVALSHA", @lua_extract_members_sha, keys.size, *keys, *argv)
rescue RedisClient::CommandError => e
raise unless e.message.start_with?("NOSCRIPT")
@lua_extract_members_sha = nil
retry
end
I believe this is our fix, but still don't fully understand how the original change was tested and with what version of sidekiq.
Can confirm, I'm experiencing this issue on Chewy 7.6.0 and Sidekiq 7.3.9
* chewy (7.6.0)
Summary: Elasticsearch ODM client wrapper
Homepage: https://github.com/toptal/chewy
Path: /Users/tyler.willingham/.asdf/installs/ruby/3.2.4/lib/ruby/gems/3.2.0/gems/chewy-7.6.0
* redis (4.8.1)
Summary: A Ruby client library for Redis
Homepage: https://github.com/redis/redis-rb
Documentation: https://www.rubydoc.info/gems/redis/4.8.1
Source Code: https://github.com/redis/redis-rb/tree/v4.8.1
Changelog: https://github.com/redis/redis-rb/blob/master/CHANGELOG.md
Bug Tracker: https://github.com/redis/redis-rb/issues
Path: /Users/tyler.willingham/.asdf/installs/ruby/3.2.4/lib/ruby/gems/3.2.0/gems/redis-4.8.1
Reverse Dependencies:
parachute-prometheus (0.12.0) depends on redis (>= 0)
* sidekiq (7.3.9)
Summary: Simple, efficient background processing for Ruby
Homepage: https://sidekiq.org
Documentation: https://github.com/sidekiq/sidekiq/wiki
Source Code: https://github.com/sidekiq/sidekiq
Changelog: https://github.com/sidekiq/sidekiq/blob/main/Changes.md
Bug Tracker: https://github.com/sidekiq/sidekiq/issues
Path: /Users/tyler.willingham/.asdf/installs/ruby/3.2.4/lib/ruby/gems/3.2.0/gems/sidekiq-7.3.9
Reverse Dependencies:
request_store-sidekiq (0.1.0) depends on sidekiq (>= 3.0)
sentry-sidekiq (5.23.0) depends on sidekiq (>= 3.0)
sidekiq-ent (7.3.4) depends on sidekiq (>= 7.3.7, < 8)
sidekiq-failures (1.0.4) depends on sidekiq (>= 4.0.0)
sidekiq-killswitch (1.1.2) depends on sidekiq (>= 3)
sidekiq-pro (7.3.6) depends on sidekiq (>= 7.3.7, < 8)
Hi there. Somehow I missed this thread. First of all, @arinhouck thank you for the patch you provided.
Here are libs versions we have:
* chewy (7.6.0)
Summary: Elasticsearch ODM client wrapper
Homepage: https://github.com/toptal/chewy
Path: ~/.rbenv/versions/3.4.1/lib/ruby/gems/3.4.0/gems/chewy-7.6.0
* redis (4.8.1)
Summary: A Ruby client library for Redis
Homepage: https://github.com/redis/redis-rb
Documentation: https://www.rubydoc.info/gems/redis/4.8.1
Source Code: https://github.com/redis/redis-rb/tree/v4.8.1
Changelog: https://github.com/redis/redis-rb/blob/master/CHANGELOG.md
Bug Tracker: https://github.com/redis/redis-rb/issues
Path: ~/.rbenv/versions/3.4.1/lib/ruby/gems/3.4.0/gems/redis-4.8.1
Reverse Dependencies:
...
brpoplpush-redis_script (0.1.3) depends on redis (>= 1.0, < 6)
redis-session-store (0.11.6) depends on redis (>= 3, < 6)
sidekiq (6.5.12) depends on redis (>= 4.5.0, < 5)
sidekiq-unique-jobs (7.1.33) depends on redis (< 5.0)
* sidekiq (6.5.12)
Summary: Simple, efficient background processing for Ruby
Homepage: https://sidekiq.org
Documentation: https://github.com/mperham/sidekiq/wiki
Source Code: https://github.com/mperham/sidekiq
Changelog: https://github.com/mperham/sidekiq/blob/main/Changes.md
Bug Tracker: https://github.com/mperham/sidekiq/issues
Path: ~/.rbenv/versions/3.4.1/lib/ruby/gems/3.4.0/gems/sidekiq-6.5.12
Reverse Dependencies:
rspec-sidekiq (5.2.0) depends on sidekiq (>= 5, < 9)
sidekiq-limit_fetch (4.4.1) depends on sidekiq (>= 6)
sidekiq-pro (5.5.8) depends on sidekiq (~> 6.0, >= 6.5.6)
sidekiq-scheduler (5.0.6) depends on sidekiq (>= 6, < 8)
sidekiq-unique-jobs (7.1.33) depends on sidekiq (>= 5.0, < 7.0)
* redis server info:
{
"redis_version" => "6.0.14",
"redis_mode" => "standalone",
"os" => "Windows ",
...
}
@skcc321 it looks like you're a major version behind on sidekiq from where I'm experiencing the issue