-
Notifications
You must be signed in to change notification settings - Fork 36
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: added support for ioredis cluster #1292
Conversation
// node bin/start-test-containers.js --redis-node-0 --redis-node-1 --redis-node-2 | ||
// docker exec -it 2aaaac7b9112 redis-cli -p 6379 cluster info | ||
module.exports = async function connect(ioredis, log) { | ||
if (!process.env.AZURE_REDIS_CLUSTER || !process.env.AZURE_REDIS_CLUSTER_PWD) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While running the tests locally, I initially encountered the following issue:
[ioredis] Unhandled error event: ClusterAllFailedError: Failed to refresh slots cache.
The issue seems to have resolved itself after some time. It might be related to the cluster since we use the same cluster for both Redis and ioredis testing. Running tests in parallel could potentially cause conflicts. We should consider alternative solutions, such as using namespaces, to prevent this issue in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We can create a card for now and do it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
c592030
to
e2cce47
Compare
refs #1327 refs #1292 We thought that this is a bug. The tests were not really clear. We revert the multi/pipeline handling for now. Raised https://jsw.ibm.com/browse/INSTA-14540 for further investigation.
refs #1327 refs #1292 We thought that this is a bug. The tests were not really clear. We revert the multi/pipeline handling for now. Raised https://jsw.ibm.com/browse/INSTA-14540 for further investigation.
refs https://jsw.ibm.com/browse/INSTA-9475