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

Add support for IORedis.Cluster type in ConnectionOptions for TypeScript #438

Merged
merged 2 commits into from
Sep 10, 2020

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Sep 8, 2020

I don't see explicitly said out that the project supports Redis cluster, neither I do see that it's been spelled out it does not work, but I do see that people talk about it, likely in javascript not in typescript that they have not encountered this problem, example #158

error TS2345: Argument of type '{ redis: IORedis.Redis | IORedis.Cluster; }' is not assignable to parameter of type 'ConnectionOptions'.
#15 5.391   Types of property 'redis' are incompatible.
#15 5.391     Type 'Redis | Cluster' is not assignable to type 'Redis | undefined'.
#15 5.391       Type 'Cluster' is missing the following properties from type 'Redis': Promise, duplicate, send_command

this PR adds { redis: IORedis.Redis | IORedis.Cluster; } typing for ConnectionOptions

cc @evantahler

@glensc glensc marked this pull request as draft September 8, 2020 13:50
@glensc glensc force-pushed the allow-redis-cluster-typescript branch from 49f0583 to c129e00 Compare September 8, 2020 15:06
@glensc
Copy link
Contributor Author

glensc commented Sep 8, 2020

However, if I monitor Redis with resque-web, I get occasionally

CROSSSLOT Keys in request don't hash to the same slot

If I read from the linked issue:

Because on cluster mode, ioredis library can not delete key on different slot at once.

so, as to my knowledge resque-web is not doing any writes just to read status, I'm assuming it's something node-resque doing.

So the problem in #158 is back?

x-ref:

@evantahler
Copy link
Member

I think the changes you have here are ready for review - they look good to me!

I can't comment how the Ruby project (resque-web) works, or it's support for redis-cluster. If you are seeing that error with node-resque, a stack trace would be very helpful!

@glensc glensc marked this pull request as ready for review September 10, 2020 08:12
@glensc
Copy link
Contributor Author

glensc commented Sep 10, 2020

I'll try to report to resque-web the problem first, but as I understand the problem comes the way things are written, but resque-web overview page should not "write" anything.

changed ready to review now, pipelines pass

@glensc
Copy link
Contributor Author

glensc commented Sep 10, 2020

But maybe it is better to introduce a new type, RedisConnection?

@glensc
Copy link
Contributor Author

glensc commented Sep 10, 2020

@evantahler evantahler merged commit ed8339f into actionhero:master Sep 10, 2020
@glensc glensc deleted the allow-redis-cluster-typescript branch September 22, 2020 03:39
@glensc
Copy link
Contributor Author

glensc commented Sep 22, 2020

@evantahler some feedback on resque-web:

It seems like the requested keys (worker ids) requested in the command aren't coming from a single node.

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