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

Detect failover from +switch-master messages #1328

Merged
merged 3 commits into from
Apr 24, 2021

Conversation

mjomble
Copy link
Contributor

@mjomble mjomble commented Apr 8, 2021

Fixes #1314

While working on this, I forgot about the lastActiveSentinel suggestion from the other issue. At this point, I'm not sure if it's needed, but I can still add it if it is.

I've tested this with various failure scenarios in a Docker cluster and it successfully reconnected in all of them. I also simulated many of the scenarios in new test cases.

@@ -39,6 +41,7 @@ export interface ISentinelConnectionOptions extends ITcpConnectionOptions {
sentinelPassword?: string;
sentinels: Array<Partial<ISentinelAddress>>;
sentinelRetryStrategy?: (retryAttempts: number) => number | void | null;
sentinelReconnectStrategy?: (retryAttempts: number) => number | void | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this name, could be confusing.

But I'm planning to submit a PR that documents all the sentinel options after this gets merged, so that might help.

@luin luin self-requested a review April 12, 2021 14:57
Copy link
Collaborator

@luin luin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive! The code looks nice! I left two suggestions, lmk your thoughts. Otherwise LGTM!

lib/connectors/SentinelConnector/types.ts Outdated Show resolved Hide resolved
lib/connectors/SentinelConnector/index.ts Outdated Show resolved Hide resolved
@mjomble
Copy link
Contributor Author

mjomble commented Apr 19, 2021

I made the suggested changes and the code got much simpler 😄
Ready for another review.

@luin luin self-requested a review April 24, 2021 09:35
@luin luin merged commit a464151 into redis:master Apr 24, 2021
@luin
Copy link
Collaborator

luin commented Apr 24, 2021

Thanks @mjomble! I just invited you as a collaborator so from now on you'll be able to approve and merge others' pull requests (which will automatically publish a new version). Feel free to participate anytime when you have time!

ioredis-robot pushed a commit that referenced this pull request Apr 24, 2021
# [4.27.0](v4.26.0...v4.27.0) (2021-04-24)

### Features

* **sentinel:** detect failover from +switch-master messages ([#1328](#1328)) ([a464151](a464151))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.27.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sentinel failover not detected when connection hangs
3 participants