-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[automatic-failover] Implement weighted endpoint selection #3519
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
base: feature/automatic-failover-1
Are you sure you want to change the base?
[automatic-failover] Implement weighted endpoint selection #3519
Conversation
- add HealthStatus
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.
Pull Request Overview
This PR introduces a HealthStatus enum and refactors the failover logic to use health-based endpoint selection instead of directly checking circuit breaker states. The changes simplify the configuration by removing the RedisDatabaseConfig inner class and having RedisDatabase accept DatabaseConfig directly.
Key Changes:
- Added
HealthStatusenum with UNKNOWN, HEALTHY, and UNHEALTHY states - Refactored
RedisDatabaseto useDatabaseConfigdirectly instead of innerRedisDatabaseConfigclass - Updated failover logic in
StatefulRedisMultiDbConnectionImplto filter by health status instead of circuit breaker state
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| HealthStatus.java | New enum defining health states (UNKNOWN, HEALTHY, UNHEALTHY) for database endpoints |
| RedisDatabase.java | Removed inner RedisDatabaseConfig class, added healthStatus field with getter, updated constructor to accept DatabaseConfig directly |
| StatefulRedisMultiDbConnectionImpl.java | Renamed getHealthyDatabase to getNextHealthyDatabase, changed filtering from circuit breaker state to health status, improved error handling with orElse(null) |
| MultiDbClientImpl.java | Simplified database creation by passing DatabaseConfig directly instead of wrapping in RedisDatabaseConfig |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/io/lettuce/core/failover/StatefulRedisMultiDbConnectionImpl.java
Show resolved
Hide resolved
src/main/java/io/lettuce/core/failover/StatefulRedisMultiDbConnectionImpl.java
Show resolved
Hide resolved
| package io.lettuce.core.failover; | ||
|
|
||
| public enum HealthStatus { | ||
| UNKNOWN, HEALTHY, UNHEALTHY |
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.
I assume UNKNOWN status is not used as of now, but is intended to be used once we have actual health checks as the initial state, till we get a result from the first health check?
| return databases.values().stream().filter(db -> db != current) | ||
| .filter(db -> db.getCircuitBreaker().getCurrentState() == CircuitBreaker.State.CLOSED) | ||
| .max(Comparator.comparingDouble(RedisDatabase::getWeight)).get(); | ||
| private RedisDatabase<C> getNextHealthyDatabase(RedisDatabase<C> current) { |
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.
Just to confirm my understanding getHealthStatus() reflects the state of database only based on HealthChecks that will be introduced(not considering the state of CB for given DB)?
In that case where do we ensure that failoverFrom() is not failing over toward DB with CB already in OPEN_STATE?
| .filter(db -> db.getCircuitBreaker().getCurrentState() == CircuitBreaker.State.CLOSED) | ||
| .max(Comparator.comparingDouble(RedisDatabase::getWeight)).get(); | ||
| private RedisDatabase<C> getNextHealthyDatabase(RedisDatabase<C> current) { | ||
| return databases.values().stream().filter(db -> db.getHealthStatus() == HealthStatus.HEALTHY) |
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.
Improve readability,I suggest something like :
private RedisDatabase<C> getNextHealthyDatabase(RedisDatabase<C> current) {
return databases.values().stream()
.filter(isHealthy)
.filter(isNotCurrent(current))
.max(DatabaseComparators.byWeight).orElse(null);
}
static class DatabaseComparators {
public static final Comparator<RedisDatabase<?>> byWeight = Comparator.comparingDouble(RedisDatabase::getWeight);
}
static class DatabasePredicates {
public static final Predicate<RedisDatabase<?>> isHealthy = db -> db.getHealthStatus() == HealthStatus.HEALTHY;
public static Predicate<RedisDatabase<?>> isNotCurrent( RedisDatabase<?> current) {
return db -> !db.equals(current);
}
}
There is half baked weighted endpoint selection in current implementation. This PR introduces the proper health status check while improves the overall quality of the code.
HealthStatusEnumRedisDatabase.RedisDatabaseConfiginner classRedisDatabasenow acceptsDatabaseConfigdirectlygetNextHealthyDatabase