-
Notifications
You must be signed in to change notification settings - Fork 454
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
Extend API to support Sidekiq #216
Comments
Thank you for bringing this issue to our attention. What is happening here is that redis-client is issuing the RESP3 handshake (HELLO 3) during its initial connection to Garnet. However, because Garnet currently does not support RESP3, this results in an error response of You should be able to circumvent this issue by using the redis gem instead, which should work with Garnet: irb(main):001:0> require 'redis'
=> true
irb(main):002:0> r = Redis.new
=> #<Redis client v5.1.0 for redis://localhost:6379>
irb(main):003:0> r.call("llen", "foo")
=> 0 |
Sidekiq 7 requires RESP3 because of the better type support. Does Garnet plan to support it? I don't see any mention on the public roadmap: |
...and the Redis 6.2 command set, things like BITFIELD_RO are used for queries. I'm not clear on which commands Garnet might be missing vs Redis 6.2. |
Ah, it looks like Garnet doesn't support blocking list operations so it's a non-starter for job systems. I hope this will change in the future. |
Which specific blocking list APIs do you need to implement a job system? We can prioritize accordingly, this isn't very hard to add - we just didn't receive a use case for it yet. |
For Sidekiq to work correctly - if you can provide us a list of missing APIs that will be quite useful. |
The Python-cousin of sidekiq, Celery, also does not work because BRPOP is not supported. With Garnet one could finally be able to use Celery on Windows Server. This matters because Django (the Python-cousin of Rails) typically uses Celery for background jobs. Thank you for this awesome project! |
Hey @mperham! Quick update - we have blocking list operations added in PR #356. |
Thank you, the other big item needed by packages like Sidekiq is the |
@TalZaccai Unfortunately I'm on macOS and can't build a custom binary since dotnet only supports windows and linux according to your getting started page. If you can point me to a Docker image for your PR, I can use that. |
Hey @mperham! You should still be able to build and run Garnet on macOS, we just haven't tested it on macOS. |
@TalZaccai Thank you for the quick implementation :) I built the branch for #356 on Windows and executed the Celery hello world. It failed because Garnet has not implemented ZREVRANGEBYSCORE. I searched the source code of Celery and found that it also depends on EVALSHA, which Garnet has not implemented. I think with those two Celery is all set with Garnet :) |
Thanks for the quick reply @mardukbp! ZREVRANGEBYSCORE should be an easy addition, unfortunately EVALSHA might take longer as we don't currently support scripting in Garnet. |
On second look, apparently EVALSHA is only a key in a dictionary, but otherwise it is not mentioned. So I guess it is not really necessary. Looking forward to trying the PR with ZREVRANGEBYSCORE! |
Alas, Sidekiq migrated to RESP3 in v7 for better datatype support. That's a showstopper. EVALSHA is not critical to Sidekiq so we can live without it (although Sidekiq Pro and Sidekiq Enterprise will not work without Lua). |
We do have basic RESP3 support in the latest. Would like to know what specific operator is not working correctly. |
I mistook an error with The Sidekiq test suite is now running with about 90% passing. With stock Redis 6.2:
With Garnet 1.0.13:
Looks like maybe there's an issue with the return value from If you install Ruby 3.x, check out the Sidekiq source (git@github.com:sidekiq/sidekiq.git), run |
Ok yeah I'm able to reproduce locally. Getting slightly less failures with the blocking ops branch, but still:
|
I'm trying to use Garnet from Ruby (I'd love to see Sidekiq running on Garnet!) and find myself blocked by case sensitivity.
Using v1.0.2.
The text was updated successfully, but these errors were encountered: