Skip to content

Conversation

mina-asham
Copy link
Contributor

  • Support for JedisSocketFactory has already been added to the lowest level Jedis to support adding any custom socket factory (e.g. UDS), this propagates the support in the JedisPool too

@mina-asham mina-asham force-pushed the pool-socket-factory branch 2 times, most recently from 8fbfdca to 5641d90 Compare November 28, 2020 19:01
@sazzad16 sazzad16 added this to the 3.4.0 milestone Nov 29, 2020
sazzad16
sazzad16 previously approved these changes Nov 29, 2020
@sazzad16 sazzad16 modified the milestones: 3.4.0, 3.5.0 Dec 7, 2020
@sazzad16 sazzad16 modified the milestones: 3.5.0, 3.6.0 Jan 19, 2021
@gkorland gkorland requested a review from dengliming March 24, 2021 08:24
@dengliming

This comment was marked as off-topic.

- Support for JedisSocketFactory has already been added to the lowest level Jedis to support adding any custom socket factory (e.g. UDS), this propagates the support in the JedisPool too
- Also fix Jedis/BinaryJedis constructors that broke after the introduction of config due to missing client initialization
@sazzad16 sazzad16 requested a review from gkorland March 25, 2021 12:45
sazzad16
sazzad16 previously approved these changes Mar 25, 2021
Copy link
Contributor

@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.

I'm not 100% about the change in

src/main/java/redis/clients/jedis/JedisSocketFactory.java

I'm leaving this discussion/debate to other reviewers.

@mp911de
Copy link
Contributor

mp911de commented Mar 25, 2021

Introducing JedisSocketFactory is a neat idea. However, it raises the question of how addressing should work. Right now, HostAndPort was the only mechanism to obtain a connection. With domain sockets, you need another way to connect to a node. I'd suggest introducing an interface and replace all usages of HostAndPort (breaking change, potentially a 4.0 topic) with that interface. Behind that interface, one can put addressing details so one could e.g. connect to Sentinel using domain sockets (or do other crazy things). You could even have an SSL implementation of JedisSocketFactory that uses SSL details from a SecureHostAndPort.

@mina-asham
Copy link
Contributor Author

Introducing JedisSocketFactory is a neat idea. However, it raises the question of how addressing should work. Right now, HostAndPort was the only mechanism to obtain a connection. With domain sockets, you need another way to connect to a node. I'd suggest introducing an interface and replace all usages of HostAndPort (breaking change, potentially a 4.0 topic) with that interface. Behind that interface, one can put addressing details so one could e.g. connect to Sentinel using domain sockets (or do other crazy things). You could even have an SSL implementation of JedisSocketFactory that uses SSL details from a SecureHostAndPort.

I think that makes sense, it will be a major overhaul though so I don't think we should block this here, having something that works for 3.x (specially that we already support that in the Jedis class) and then we work on something bigger for 4.x

@sazzad16 sazzad16 force-pushed the pool-socket-factory branch from f472d8c to c939b78 Compare March 28, 2021 06:41
@mina-asham mina-asham requested review from gkorland and sazzad16 March 28, 2021 09:16
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.

5 participants