Add Unix domain socket support #1942
Conversation
mina-asham
force-pushed
the
unix-domain-socket
branch
4 times, most recently
from
12d1afc to
86835f4
Compare
mina-asham
force-pushed
the
unix-domain-socket
branch
from
86835f4 to
0c35deb
Compare
|
@sazzad16 @marcosnils @gkorland can you have a look? |
| </dependency> | ||
| <dependency> | ||
| <groupId>com.kohlschutter.junixsocket</groupId> | ||
| <artifactId>junixsocket-native-common</artifactId> |
There was a problem hiding this comment.
I don't like the idea of adding one more dependency* just to support one feature.
* compile time
There was a problem hiding this comment.
Yeah, I wasn't too mad about it as well, but thought it would be fine since it's a small dependency. So I think we have a few options (ordered by most to least favourite by me, but open to suggestions):
- [My preferred suggestion] Make the dependency an optional one and do a reflection check to see if it is and if the feature can be supported
- Shade and include the dependency (also since it's tiny), this way it would be self included and no need for consumers to worry
- Accept the dependency as is now since it's tiny
- Not include it at all and follow your below comments passing a socket straight in the constructor. It's my least preferred because I think the consumer shouldn't need to worry about creating/configuring the socket, specially that Jedis has some good defaults there. In the below comment you mention other sockets, any specific ones in mind?
(Sorry for the delayed response, this escaped me somehow...)
| this.host = host; | ||
| } | ||
|
|
||
| public Connection(final File unixDomainSocket) { |
There was a problem hiding this comment.
What if we take the Socket (and/or other objects) or a custom object which would create/connect a socket on demand? This would help to support many forms of sockets to be supported by Jedis at once.
There was a problem hiding this comment.
See last point in above comment.
There was a problem hiding this comment.
you mention other sockets, any specific ones in mind?
Not anything specific, rather generic.
I just meant, if we had such constructor, sockets like unix domain socket, ssl or even plain socket with different configuration from Jedis could be used by users.
There was a problem hiding this comment.
I just meant, if we had such constructor, sockets like unix domain socket, ssl or even plain socket with different configuration from Jedis could be used by users.
We can do that, but I think the question is should we expose that to the user as part of the library?
Jedis tries to do such constructs under the hood and expose other options (ssl, passwords, etc...) and sets some defaults for each of these, we would risk exposing this and making the library a bit harder (or a bit more confusing) to use.
It doesn't make much difference to my case by the way, so I am not opposing this, I just want to make sure you would be okay with that (minimal) risk.
There was a problem hiding this comment.
we would risk exposing this and making the library a bit harder (or a bit more confusing) to use.
I'm not sure I follow you.
There was a problem hiding this comment.
As a user, I would much rather set a minimal parameters (e.g. port number, UDS path, credentials), than creating/setting/maintaining a socket (more code and might not be setup with the best configuration unless I am an advanced user).
Not the strongest argument, but this is my intuition. What do you think?
There was a problem hiding this comment.
-
About dependency:
Most users are not enthusiastic about more dependencies. So I'm a bit reluctant on this. -
About not easy option:
At least it is an option which would allow avid users to avoid long routes (like extending Jedis/Connection classes, building custom Jedis). Let's takesendCommandmethod (coming in 3.1.0) as an example. It would require some coding for the user but would allow them to use new commands in Redis (which are not in Jedis) more easily without waiting for Jedis to include those.
These are my viewpoints.
There was a problem hiding this comment.
Alright, sounds good, I will refactor this to allow creating it with a Socket object for advanced use-cases, will update this soon :)
…main Socket) or any other custom address resolution - Related pull requests: redis#2036 and redis#1942 and redis#1132
|
Resolved by #2151 |
2 participants
Fixes issue #492, and revives pull request #1132
The code is mostly based on the previous pull request with one main modification: exposing the unix domain socket file in the constructor. This makes the interface more clear from a Jedis client point of view.
Note: Tests were already failing due to latest Redis unstable version, but the test for this commit is working fine, and all tests pass on Redis 5.0 branch.