Skip to content

Conversation

keddar1
Copy link

@keddar1 keddar1 commented Jul 17, 2019

When I use redis-server with sentinel I can not use safe connect to redis-server with ssl because sentinel Pool in default create not safe connection.

@gkorland
Copy link
Contributor

#1987

@gkorland gkorland requested a review from sazzad16 July 17, 2019 14:43
@keddar1 keddar1 changed the title Allowed connect to redis-master Allowed connect to redis-master with ssl. Jul 18, 2019
@@ -35,6 +35,11 @@
this(host, port, connectionTimeout, soTimeout, password, database, clientName,
false, null, null, null);
}
JedisFactory(final String host, final int port, final int connectionTimeout,
final int soTimeout, final String password, final int database, final String clientName, final boolean isRedisSslEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this constructor? Isn't following constructor enough?

I'm asking this because JedisFactory is kept to have fewest constructors.

public class JedisSentinelPool extends JedisPoolAbstract {

protected GenericObjectPoolConfig poolConfig;

protected int connectionTimeout = Protocol.DEFAULT_TIMEOUT;
protected int soTimeout = Protocol.DEFAULT_TIMEOUT;

Copy link
Contributor

Choose a reason for hiding this comment

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

An empty line would make code more readable.

@gkorland
Copy link
Contributor

I think we should have a test for this use case

@gkorland gkorland added this to the 3.2.0 milestone Jul 24, 2019
@gkorland gkorland modified the milestones: 3.2.0, 3.3.0 Nov 25, 2019
@sazzad16 sazzad16 modified the milestones: 3.3.0, 3.4.0 Mar 14, 2020
@sazzad16 sazzad16 modified the milestones: 3.4.0, 3.5.0 Nov 24, 2020
@sazzad16 sazzad16 modified the milestones: 3.5.0, 3.6.0 Jan 19, 2021
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.

3 participants