Skip to content

feat(ring): specify custom health check func via HeartbeatFn option #2940

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

Merged
merged 11 commits into from
Aug 5, 2025

Conversation

strobil
Copy link
Contributor

@strobil strobil commented Mar 13, 2024

The current implementation of the Redis ring does not support specifying custom shard health check rules.
We use a Redis shard to distribute the load across multiple Redis slave instances.
However, if any shard encounters replication issues, the Redis ring continues to utilize this shard because it responds to ping requests successfully.
Thus, the Redis ring does not remove the malfunctioning shard, even though it is inconsistent.

This pull request proposes adding a HeartbeatFn option to the RingOptions, which can be configured during ring construction. This addition enables the definition of custom health check logic, such as checks based on the current replication status of the shard.

@strobil strobil marked this pull request as draft March 13, 2024 22:19
@strobil strobil marked this pull request as ready for review March 14, 2024 10:42
@strobil strobil changed the title specify custom health check func via ShardHealthCheckFn option specify custom health check func via HeartbeatFn option Mar 14, 2024
@ndyakov
Copy link
Member

ndyakov commented Feb 4, 2025

Thank you, @strobil! I do think having a custom healthcheck in the ring can benefit the users. Would you mind resolving conflicts so we can merge master and proceed with review of this PR?

@strobil strobil changed the title specify custom health check func via HeartbeatFn option feat(ring): specify custom health check func via HeartbeatFn option Feb 21, 2025
@strobil
Copy link
Contributor Author

strobil commented Feb 21, 2025

@ndyakov
Hi, all merge conflicts were resolved!

@ndyakov
Copy link
Member

ndyakov commented Aug 4, 2025

@strobil i do see the test continue to fail, would you mind taking a look

@ndyakov ndyakov self-requested a review August 5, 2025 11:59
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Approving this. @strobil keep in mind we are not actively supporting ring since there are other limitation that simply cannot be covered with clustering controlled from the client.

@ndyakov ndyakov merged commit 7158a8d into redis:master Aug 5, 2025
20 checks passed
ofekshenawa added a commit that referenced this pull request Aug 10, 2025
…2940)

* specify custom health check func via ShardHealthCheckFn option

* ShardHealthCheckFn renamed to HeartbeatFn

---------

Co-authored-by: Mykhailo Alipa <strobil@Mykhailos-MacBook-Air.local>
Co-authored-by: ofekshenawa <104765379+ofekshenawa@users.noreply.github.com>
Co-authored-by: Nedyalko Dyakov <nedyalko.dyakov@gmail.com>
Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants