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

Cross cluster failover #3310

Merged
merged 38 commits into from
May 26, 2023
Merged

Cross cluster failover #3310

merged 38 commits into from
May 26, 2023

Conversation

uglide
Copy link
Contributor

@uglide uglide commented Mar 3, 2023

No description provided.

Allen Terleto added 4 commits February 23, 2023 21:09
- Created new CircuitBreakerCommandExecutor to leverage new retry and circuit breaker capability for failover
- Created new MultiClusterJedislientConfig to encapsulate resilience4j configurations
- Created new MultiClusterPooledConnectionProvider to encapsulate multi-cluster management and operational capabilities
- Created new JedisValidationException
- Added new constructor to UnifiedJedis
- Added resilience4j to pom.xml
- Changed CircuitBreakerCommandExecutor to be more thread safe by passing cluster by reference instead of multiple lookups
- Exposed MultiClusterPooledConnectionProvider.Cluster as public so it can be accessed and passed by reference within CircuitBreakerCommandExecutor
- Made some javadocs updates for easier readability
- Removed debug from happy path so it has parity with other executors. It would likely be too busy on the logs in a production system anyway
- Moved log responsibility into provider for consistency
- Added logging for manual failback/failover with consistent wording to the automated failover
- Provided a better log for when the prioritized list is exhausted
- Added more safety for orchestration within mutation-operations on the activeMultiClusterIndex to avoid edge cases. In practice this will never likely come up but better to be extra careful as to avoid a deadlock or inaccurate transitions
@uglide uglide requested review from sazzad16 and chayim March 3, 2023 12:59
@sazzad16
Copy link
Collaborator

sazzad16 commented Mar 4, 2023

@uglide I may take some time (days) to do proper review.

BTW, are we going to do this without tests?

@yangbodong22011 @dengliming @mina-asham Please participate if you could. Thanks!

Copy link
Contributor

@dengliming dengliming left a comment

Choose a reason for hiding this comment

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

Just a quick look, I think we also need to write unit tests to cover these new feature changes.

pom.xml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023

Codecov Report

Patch coverage: 65.71% and project coverage change: -0.06 ⚠️

Comparison is base (125ee24) 68.09% compared to head (e01b6ae) 68.04%.

❗ Current head e01b6ae differs from pull request most recent head 0851cfd. Consider uploading reports for the commit 0851cfd to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3310      +/-   ##
============================================
- Coverage     68.09%   68.04%   -0.06%     
- Complexity     4498     4541      +43     
============================================
  Files           271      275       +4     
  Lines         14352    14597     +245     
  Branches        960      978      +18     
============================================
+ Hits           9773     9932     +159     
- Misses         4180     4252      +72     
- Partials        399      413      +14     
Impacted Files Coverage Δ
...nts/jedis/exceptions/JedisValidationException.java 33.33% <33.33%> (ø)
.../redis/clients/jedis/MultiClusterClientConfig.java 64.58% <64.58%> (ø)
...roviders/MultiClusterPooledConnectionProvider.java 66.07% <66.07%> (ø)
...jedis/executors/CircuitBreakerCommandExecutor.java 66.66% <66.66%> (ø)
...rc/main/java/redis/clients/jedis/UnifiedJedis.java 71.62% <100.00%> (+0.19%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@yangbodong22011
Copy link
Collaborator

Is there an issue discussion that already exists before your PR that gave me the background for this PR.

@sazzad16
Copy link
Collaborator

sazzad16 commented Mar 14, 2023

Is there an issue discussion that already exists before your PR that gave me the background for this PR.

@yangbodong22011 Sadly, no.

I'm writing from my own: This PR is to communicate with multiple Redis servers which are set in multiple geo-locations and are eventually consistent. The aim is to choose the fastest and most reliable server for the (java) application.

@aterleto @uglide Please correct/improve if I'm wrong.

Allen Terleto added 4 commits April 6, 2023 16:17
…lable

- Moved increment below a validation so subsequent calls to lookup the cluster connection do not throw a nullpointer exception
- Replaced custom connection close logic with try-with-resources statements
…possible

- Added logic to fallback method that handles subsequent calls after all failover attempts have been exhausted and only a manual failback can resume operations
- Added new flag to indicate that all attempts to failover have been exhausted
- Changed comments to clarify that an endpoint can belong to a cluster but also a database so it is more OSS friendly
- Added logic to the manual failback method to allow an existing cluster to reattempt to connect to its current cluster/database in case its the only option that became available
- Users can now configure their custom logic to persist the activeMultiClusterIndex or custom logging after a successful cluster failover via a functional interface
@sazzad16 sazzad16 dismissed their stale review April 8, 2023 09:51

just dismiss

@sazzad16
Copy link
Collaborator

sazzad16 commented Apr 8, 2023

@aterleto @uglide Please resolve the conflicts with the master branch. I have created aterleto#2 for your convenience.

banker and others added 5 commits April 10, 2023 23:48
Co-authored-by: Kyle Banker <banker@users.noreply.github.com>
Co-authored-by: Kyle Banker <banker@users.noreply.github.com>
pom.xml Show resolved Hide resolved
docs/failover.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

@aterleto Please at least accept these change requests and help us bring less confusion between this implementation and long time existing JedisCluster.

@aterleto
Copy link
Contributor

@aterleto Please at least accept these change requests and help us bring less confusion between this implementation and long time existing JedisCluster.

Completed as requested

@sazzad16 sazzad16 added this to the 5.0.0 milestone May 26, 2023
@sazzad16 sazzad16 merged commit 967cceb into redis:master May 26, 2023
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.

8 participants