ADD UDS support

[sunheehnus]

No description provided.

[marcosnils]

@sunheehnus tests fail for JDK 1.6. Seems like the junixsocket dependency added is compiled only for 1.7 and above. Can we see if 1.6 can be supported also?. There are still many Jedis clients that use this JDK version.

[marcosnils]

Please keep old file format. Re-formatting has been applied

[sunheehnus]

OK, will do, but there is a mix of tabs and spaces in old file.

[marcosnils]

@sunheehnus thanks for this awesome contribution. We've been willing to add UDS to Jedis for a long time and we haven't find a proper approach to do it. I like your PR as it keeps everything clean and simple. There are some comments which I would like to review before merging.

@xetorthio @HeartSaVioR @allanwax @christophstrobl ping.

[sunheehnus]

Thanks for your quick reply @marcosnils.

tests fail for JDK 1.6. Seems like the junixsocket dependency added is compiled only for 1.7 and above. Can we see if 1.6 can be supported also?. There are still many Jedis clients that use this JDK version.

junixsocket 1.3 can support JDK 1.6, but junixsocket 1.3 is not found in Maven Central.
I can use the following patch to pom.xml, and it passes the test. :-)

diff --git a/pom.xml b/pom.xml
index 5808306..fd95e78 100644
--- a/pom.xml
+++ b/pom.xml
@@ -68,9 +68,9 @@
                        <scope>compile</scope>
                </dependency>
                <dependency>
-                       <groupId>com.kohlschutter.junixsocket</groupId>
-                       <artifactId>junixsocket-common</artifactId>
-                       <version>2.0.4</version>
+                       <groupId>org.newsclub.net</groupId>
+                       <artifactId>junixsocket</artifactId>
+                       <version>1.3</version>
                </dependency>
                <dependency>
                        <groupId>com.kohlschutter.junixsocket</groupId>
@@ -85,6 +85,13 @@
                </dependency>
        </dependencies>

+    <repositories>
+        <repository>
+            <id>vt-middleware</id>
+            <name>VT Middleware GitHub Repository</name>
+            <url>https://raw.github.com/vt-middleware/maven-repo/master</url>
+        </repository>
+    </repositories>
        <distributionManagement>
                <repository>
                        <id>github</id>

[HeartSaVioR]

All comments I want to say were addressed by @marcosnils.
Only one thing I want to know is that this PR shouldn't affect Windows environment.
@sunheehnus Did you test this PR in Windows machine?

[sunheehnus]

Hi @HeartSaVioR , I just test it on a Windows machine, using the service of RedisLabs'.
Tests have passed.
But there is one thing that when we generate a jedis.jar, we should include the depencies of junixsocket*.jar & junixsocket*.nar, etc.

I add the following to the pom.xml

<plugin>  
  <artifactId>maven-assembly-plugin</artifactId>  
  <configuration>  
    <descriptorRefs>  
      <descriptorRef>jar-with-dependencies</descriptorRef>  
    </descriptorRefs>  
  <archive>  
    <manifest>  
      <mainClass></mainClass>  
    </manifest>  
  </archive>  
  </configuration>  
  <executions>  
    <execution>  
      <id>make-assembly</id>  
      <phase>package</phase>  
      <goals>  
        <goal>single</goal>  
      </goals>  
   </execution>  
  </executions>  
</plugin>  

It can generate the jedis-jar-with-dependencies.jar.
I am not familiar with maven indeed, maybe there is some better way. :-)

[HeartSaVioR]

If we decide that Jedis should provide assembled jar (fat/uber jar), we can consider about shading dependencies.
I'm curious that how maven-nar-plugin works, since there're many xNix OSes and its native libraries may be not compatible.
When we confirm that uber jar runs without any issues from Linux, OSX, Windows, I'll be OK to merge this.

[nikolaikopernik]

ping guys. Really important PR

[marcosnils]

@sunheehnus ping?

[sunheehnus]

Hi @marcosnils , I will do some clean as you suggested and squash the commits to one ASAP, just sent an email to the creator of junixsocket to ask for some hints about the JDK 1.6 :-)

[sunheehnus]

Hi @marcosnils , there is a trade off to solve the JDK 1.6 issue:

• I compile the junixsocket locally
• add the junixsocket-common-2.0.4.jar to jedis

This could be the most clean way to using junixsocket IMHO.
Maybe we can wait for the official support of JDK 1.6, but I am not sure if this is in the plan of official junixsocket.
Could I have your opinion? Thanks :-)

[marcosnils]

@sunheehnus we're planning to drop support for Java 1.6 and older from jedis 3.0 onwards. I'll remove older JVM support in CI master branch so you can run tests accordingly. There are still pending comments to resolve in this PR, can you please rebase and check the pending items?

Thanks!.

[sunheehnus]

Hello @marcosnils , that's great, the new commit will be pushed today. :-)

[sunheehnus]

Hello @marcosnils , I have already fixed the issues you suggested, and squashed the commits to a single one.

There are some more informations I want to share:

• The junixsocket is now have some issues using setTcpNoDelay, hope in the next version of junixsocket, we can add this configuration.
• I move the connect before set configurations, because in junixsocket(maybe I am wrong?), we must set configuraton after the connect.
• I make an assumption that UDS address must be ABSOLUTE address, make sense?

Thanks very much. :-)

[marcosnils]

I'm wondering if we might be hitting some performance issue when running this condition every time Jedis tries to establish a connection and needs to parse the supplied host.

@allanwax @HeartSaVioR @xetorthio @sunheehnus. WDYT?

[sunheehnus]

What about just check if it is an ABS path?

[allanwax]

It's easy to avoid the recheck each time. Keep a static ConcurrentHashMap of the string and whether it is 'ok'. On the first reference do the lookup, store the results, and keep going. On subsequent lookups, read from the hash map. If it's there, you're done.

If you are betting that the status of each of these hosts will change over time, start up a Timer which clears the map every so often, as determined by your level of paranoia. So every so often a new check will be done.

[marcosnils]

What about just check if it is an ABS path?
Maybe it could also be a relative path to the CWD?

It's easy to avoid the recheck each time. Keep a static ConcurrentHashMap of the string and whether it is 'ok'. On the first reference do the lookup, store the results, and keep going. On subsequent lookups, read from the hash map. If it's there, you're done.

If you are betting that the status of each of these hosts will change over time, start up a Timer which clears the map every so often, as determined by your level of paranoia. So every so often a new check will be done.

Seems a bit unnecessary as connect shouldn't happen that often in the code. I was looking for an alternative to check the supplied path. I don't care if the file exists because the UDS library already checks this and throws a proper exception (just realized that we're not testing this).

[sunheehnus]

Hello @marcosnils , if we use a relative path, the current approach this patch implement may have a potential bug that if the path is already included in /etc/hosts, then we may open a TCP connection instead, maybe we should change the API or just keep it simple using the abs path?

[dmandalidis]

ping

[mina-asham]

Ping, would quite like to use this. @sunheehnus I am happy to revive this in another pull request if you don't have time?
This would fix: #492

[sazzad16]

Resolved by #2151