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

feat: allow custom Redis-like broker #976

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asdfhuang
Copy link

This PR decouples Broker interface from redis and allows creating Server and Client directly from a broker instance. Which makes it possible for custom brokers.

For exmaple: Redis from our infrastructure doesn't support pubsub. But I can inherit RDB and implement the pub/sub in another way to make asynq work without a huge amount of work.

@asdfhuang
Copy link
Author

asdfhuang commented Dec 3, 2024

There are some known issues for this PR:

  • Inspector and Scheduler are locked to redis broker now, they might not work if we use custom borkers.
  • Using type alias to export Broker types from internal package is a temporary solution.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.37%. Comparing base (4f00f52) to head (6dad2eb).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
- Coverage   67.13%   66.37%   -0.77%     
==========================================
  Files          29       29              
  Lines        4300     5749    +1449     
==========================================
+ Hits         2887     3816     +929     
- Misses       1135     1652     +517     
- Partials      278      281       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kamikazechaser kamikazechaser changed the title feat: allow custom broker feat: allow custom Redis-like broker Dec 4, 2024
@kamikazechaser
Copy link
Collaborator

What redis-like broker is this? Is it one of the forks or is it one that implements the Redis wire protocol?

@kamikazechaser
Copy link
Collaborator

kamikazechaser commented Dec 4, 2024

The idea is good. Though we wouldn't land this in master immediately because:

  1. We would need to move the types into a publicly importable folder e.g. pkg. I like the short term fix, but it makes this library look hacky. We have already introduced some hacky bits which are starting to show (see fix: NewScheduler incorrectly creates underlying Client, closing broker properly #977).
  2. Work on supporting the scheduler and inspector to use the custom Broker (if possible at all).
  3. We have the problem of go-redis which is now transferred to Redis corp. I'm not even sure if there will be guaranteed support for redis forks and clones (https://news.ycombinator.com/item?id=42239607). (Though I see you changes to Client kind of address this, but I'm not sure it addresses everywhere else in the library which ends up using go-redis, repeation of my 2nd point)

@asdfhuang
Copy link
Author

What redis-like broker is this? Is it one of the forks or is it one that implements the Redis wire protocol?

It's a redis fork that maintained and operated by our infra team. The problem is that there's always a proxy in front of the redis instances, and the proxy doesn't support pubsub.

  1. We would need to move the types into a publicly importable folder e.g. pkg. I like the short term fix, but it makes this library look hacky. We have already introduced some hacky bits which are starting to show (see fix: NewScheduler incorrectly creates underlying Client, closing broker properly #977).
  2. Work on supporting the scheduler and inspector to use the custom Broker (if possible at all).
  3. We have the problem of go-redis which is now transferred to Redis corp. I'm not even sure if there will be guaranteed support for redis forks and clones (https://news.ycombinator.com/item?id=42239607). (Though I see you changes to Client kind of address this, but I'm not sure it addresses everywhere else in the library which ends up using go-redis, repeation of my 2nd point)

for the 1st& 2nd point, I'll try to upload a new patch to resolve them.

@kamikazechaser
Copy link
Collaborator

It's a redis fork

Ah ok. It should generally be fine for now. I have started a discussion around this in #981.

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 this pull request may close these issues.

2 participants