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

Add active detection in sentinel mode,Reconstruct sentinel mode Listener #3547

Closed
wants to merge 21 commits into from
Closed

Conversation

chenshi5012
Copy link
Contributor

  1. Add active detection in sentinel mode,
  2. Reconstruct sentinel mode Listener
  3. Add test case for master listener for sentinel

sentinel mode, lost "+switch-master" message, should client add watchdog thread #3525
please check this, JedisSentinelPool / SentineledConnectionProvider provider monitor master-slave failover by subscribe the message from “+switch-master”。

this pr modify master-slave failover monitor,Two listening mechanisms are provided, one is the default subscription mechanism and the other is the active detection mechanism;
When due to network reasons, the client loses the subscription message and the main node connection of the jedis factory is still not switched, it will cause the connection to be unavailable.

  1. SentinelMasterSubscribeListener the default listener
  2. SentinelMasterActiveDetectListener the active detect listener

you can specify which listener to use; or open both. default is SubscribeListener,
`

SentinelPoolConfig config = new SentinelPoolConfig();
config.setEnableActiveDetectListener(true);   // default off 
config.setEnableDefaultSubscribeListener(true); // default on 
config.setActiveDetectIntervalTimeMillis(5*1000); // default 5s 
config.setSubscribeRetryWaitTimeMillis(5*1000); // default  5s 

JedisSentinelPool pool = new JedisSentinelPool(MASTER_NAME, sentinels, config, 1000,
        "foobared", 2);

`

…ner,Add test case for master listener for sentinel

Signed-off-by: c00603587 <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (4cd5a6a) 71.49% compared to head (33e2028) 71.58%.
Report is 4 commits behind head on master.

❗ Current head 33e2028 differs from pull request most recent head 465e5ba. Consider uploading reports for the commit 465e5ba to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3547      +/-   ##
============================================
+ Coverage     71.49%   71.58%   +0.09%     
- Complexity     4847     4875      +28     
============================================
  Files           288      291       +3     
  Lines         15450    15507      +57     
  Branches       1093     1099       +6     
============================================
+ Hits          11046    11101      +55     
+ Misses         3927     3923       -4     
- Partials        477      483       +6     
Files Coverage Δ
src/main/java/redis/clients/jedis/Connection.java 81.17% <ø> (-1.26%) ⬇️
...n/java/redis/clients/jedis/SentinelPoolConfig.java 100.00% <100.00%> (ø)
...in/java/redis/clients/jedis/JedisSentinelPool.java 73.94% <96.15%> (+8.58%) ⬆️
...c/main/java/redis/clients/jedis/JedisMetaInfo.java 68.75% <40.00%> (ø)
.../main/java/redis/clients/jedis/CommandObjects.java 85.65% <88.88%> (-0.13%) ⬇️
...entinel/listener/SentinelActiveDetectListener.java 82.05% <82.05%> (ø)
.../jedis/providers/SentineledConnectionProvider.java 84.21% <65.21%> (+8.02%) ⬆️
...s/sentinel/listener/SentinelSubscribeListener.java 57.14% <57.14%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chenshi5012
Copy link
Contributor Author

@sazzad16 thanks for your suggest . please take a look for this PR.

@sazzad16
Copy link
Collaborator

@chenshi5012 Have you made any change after your previous PR?

Moreover, your tests are still @Ignore.

c00603587 added 15 commits October 19, 2023 10:32
* 'master' of https://github.com/chenshi5012/jedis: (21 commits)
  Bump org.json:json from 20230618 to 20231013 (#3586)
  Fix SORTABLE argument issue in FT.CREATE command (#3584)
  Linking to Redis resources (#3583)
  Remove patch version snapshots
  Make integration and snapshot actions manually triggerable (#3579)
  Missing piece from #3578
  Release minor version snapshots (#3578)
  Allow getting schema field name (#3576)
  Fix binary variants of XRANGE and XREAD commands (#3571)
  Support GEOSHAPE field type in RediSearch (#3561)
  Bump org.apache.commons:commons-pool2 from 2.11.1 to 2.12.0 (#3565)
  Bump junixsocket-core to 2.8.1 (#3573)
  Bump org.apache.maven.plugins:maven-javadoc-plugin from 3.5.0 to 3.6.0 (#3539)
  Broadcast FUNCTION LOAD command methods (#3557)
  Encode map in encoded object (#3555)
  Polish Triggers and functions tests (#3554)
  Different variable names for json v1 and v2 interfaces (#3553)
  Fix Search/Gears test regression (#3552)
  Polish Feature: Triggers and functions commands (#3551)
  Feature: Triggers and functions commands (#3531)
  ...
Signed-off-by: c00603587 <[email protected]>
Signed-off-by: c00603587 <[email protected]>
Signed-off-by: c00603587 <[email protected]>
1
Signed-off-by: c00603587 <[email protected]>
This reverts commit 5cd40fc.

# Conflicts:
#	src/test/java/redis/clients/jedis/examples/GeoShapeFieldsUsageInRediSearch.java

Signed-off-by: c00603587 <[email protected]>
@chenshi5012
Copy link
Contributor Author

@chenshi5012 Have you made any change after your previous PR?

Moreover, your tests are still @Ignore.

ignore has beenremove and the test case move to JedisSentinelTest.java

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.

Many new classes only for sentinel. May consider a new package.


import org.apache.commons.pool2.impl.GenericObjectPoolConfig;

public class SentinelPoolConfig extends GenericObjectPoolConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

GenericObjectPoolConfig takes a type argument. Declaring a sub-class without the type argument is anti-pattern.

Idea: Just declare this interface separately without sub-classing it; with a more appropriate name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be sub-class is the best way to avoid add more Constructor method . currently threse are 24 Constructor method for JedisSentinelPool.
use sub-class will not break the Constructor parameter type GenericObjectPoolConfig.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that's your concern, you can introduce a Builder.

* @see SentinelMasterSubscribeListener Passive subscription
* @see SentinelMasterActiveDetectListener Active detection
*/
public interface SentinelMasterListener {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check comment in implementing classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

src/test/java/redis/clients/jedis/JedisSentinelTest.java Outdated Show resolved Hide resolved
@sazzad16
Copy link
Collaborator

@chenshi5012 This is a not so simple PR. I already have too many things in my regular job. So, would take time to properly review this. However, added some quick comments.

c00603587 added 4 commits November 1, 2023 10:35
Signed-off-by: c00603587 <[email protected]>

(cherry picked from commit 23813a2)
Signed-off-by: c00603587 <[email protected]>
Signed-off-by: c00603587 <[email protected]>
Signed-off-by: c00603587 <[email protected]>
@chenshi5012
Copy link
Contributor Author

Many new classes only for sentinel. May consider a new package.

thanks for your pantinet , new package "redis.clients.jedis.sentinel.listener"

@yangbodong22011
Copy link
Collaborator

As far as I know, SentinelListener#run will always run and possibly reinitialize sentinelMaster:

  1. Receive "+switch-master"
  2. After receiving JedisException, it will be reinitialized after sleep subscribeRetryWaitTimeMillis.

If your client does not recover in either case, what could be the reason?

@chenshi5012
Copy link
Contributor Author

As far as I know, SentinelListener#run will always run and possibly reinitialize sentinelMaster:

  1. Receive "+switch-master"
  2. After receiving JedisException, it will be reinitialized after sleep subscribeRetryWaitTimeMillis.

If your client does not recover in either case, what could be the reason?

actually ,active detect will enhanced stability. while client lost subscribe message

image

@yangbodong22011
Copy link
Collaborator

@chenshi5012 Yes, I know Pubsub is unreliable, and I'd like to know what specific issues you're experiencing in production?

pubsub messages lost? Or is the client always in a silent state and unable to sense that the link is disconnected?

@chenshi5012
Copy link
Contributor Author

Yes, in Sentinel mode, we've had this problem many times in the production environment.

  1. The data load is heavy. (80G+, flush every 30mins )
  2. the network not stable (client in other vpc. ping-latancy Occasionally very high )
  3. The node breaks down unexpectedly (the server and sentinel are together in one node ), triggering the active/standby switchover.

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

Successfully merging this pull request may close these issues.

4 participants