Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,18 @@ cluster-enabled yes
cluster-config-file /tmp/redis_cluster_node6.conf
endef

# UDS REDIS NODES
define REDIS_UDS
daemonize yes
port 0
pidfile /tmp/redis_uds.pid
logfile /tmp/redis_uds.log
unixsocket /tmp/redis_6379.sock
unixsocketperm 777
save ""
appendonly no
endef

#STUNNEL
define STUNNEL_CONF
cert = src/test/resources/private.pem
Expand All @@ -228,6 +240,7 @@ export REDIS_CLUSTER_NODE3_CONF
export REDIS_CLUSTER_NODE4_CONF
export REDIS_CLUSTER_NODE5_CONF
export REDIS_CLUSTER_NODE6_CONF
export REDIS_UDS
export STUNNEL_CONF
export STUNNEL_BIN

Expand Down Expand Up @@ -258,6 +271,7 @@ start: stunnel cleanup
echo "$$REDIS_CLUSTER_NODE4_CONF" | redis-server -
echo "$$REDIS_CLUSTER_NODE5_CONF" | redis-server -
echo "$$REDIS_CLUSTER_NODE6_CONF" | redis-server -
echo "$$REDIS_UDS" | redis-server -

cleanup:
- rm -vf /tmp/redis_cluster_node*.conf 2>/dev/null
Expand Down Expand Up @@ -285,6 +299,7 @@ stop:
kill `cat /tmp/redis_cluster_node4.pid` || true
kill `cat /tmp/redis_cluster_node5.pid` || true
kill `cat /tmp/redis_cluster_node6.pid` || true
kill `cat /tmp/redis_uds.pid` || true
kill `cat /tmp/stunnel.pid` || true
rm -f /tmp/sentinel1.conf
rm -f /tmp/sentinel2.conf
Expand Down
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<redis-hosts>localhost:6379,localhost:6380,localhost:6381,localhost:6382,localhost:6383,localhost:6384,localhost:6385</redis-hosts>
<sentinel-hosts>localhost:26379,localhost:26380,localhost:26381</sentinel-hosts>
<cluster-hosts>localhost:7379,localhost:7380,localhost:7381,localhost:7382,localhost:7383,localhost:7384,localhost:7385</cluster-hosts>
<uds-hosts>/tmp/redis_6379.sock</uds-hosts>
<github.global.server>github</github.global.server>
</properties>

Expand All @@ -66,6 +67,11 @@
<type>jar</type>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.kohlschutter.junixsocket</groupId>
<artifactId>junixsocket-native-common</artifactId>
<version>2.0.4</version>
</dependency>
</dependencies>

<distributionManagement>
Expand Down
31 changes: 23 additions & 8 deletions src/main/java/redis/clients/jedis/Connection.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package redis.clients.jedis;

import java.io.File;
import java.io.Closeable;
import java.io.IOException;
import java.net.InetSocketAddress;
Expand All @@ -8,6 +9,9 @@
import java.util.ArrayList;
import java.util.List;

import org.newsclub.net.unix.AFUNIXSocket;
import org.newsclub.net.unix.AFUNIXSocketAddress;

import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLParameters;
import javax.net.ssl.SSLSocket;
Expand Down Expand Up @@ -38,6 +42,11 @@ public class Connection implements Closeable {
private SSLParameters sslParameters;
private HostnameVerifier hostnameVerifier;

private boolean isUDSConnection(String host) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

What about just check if it is an ABS path?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

File udsFile = new File(host);
return udsFile.isAbsolute() && udsFile.exists();
}

public Connection() {
}

Expand Down Expand Up @@ -167,19 +176,24 @@ public void setPort(final int port) {
public void connect() {
if (!isConnected()) {
try {
socket = new Socket();
// ->@wjw_add
socket.setReuseAddress(true);
if (isUDSConnection(host)) {
socket = AFUNIXSocket.newStrictInstance();
socket.connect(new AFUNIXSocketAddress(new File(host)), connectionTimeout);
} else {
socket = new Socket();
socket.connect(new InetSocketAddress(host, port), connectionTimeout);
// ->@wjw_add
socket.setReuseAddress(true);
socket.setTcpNoDelay(true); // Socket buffer Whetherclosed, to
// ensure timely delivery of data
}
socket.setKeepAlive(true); // Will monitor the TCP connection is
// valid
socket.setTcpNoDelay(true); // Socket buffer Whetherclosed, to
// ensure timely delivery of data
socket.setSoLinger(true, 0); // Control calls close () method,
// the underlying socket is closed
// immediately
// <-@wjw_add

socket.connect(new InetSocketAddress(host, port), connectionTimeout);
socket.setSoTimeout(soTimeout);

if (ssl) {
Expand Down Expand Up @@ -227,8 +241,9 @@ public void disconnect() {
}

public boolean isConnected() {
return socket != null && socket.isBound() && !socket.isClosed() && socket.isConnected()
&& !socket.isInputShutdown() && !socket.isOutputShutdown();
return socket != null && (socket instanceof AFUNIXSocket ? true : socket.isBound()) &&
!socket.isClosed() && socket.isConnected() && !socket.isInputShutdown() &&
!socket.isOutputShutdown();
}

public String getStatusCodeReply() {
Expand Down
22 changes: 22 additions & 0 deletions src/test/java/redis/clients/jedis/tests/HostAndPortUtil.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package redis.clients.jedis.tests;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import redis.clients.jedis.HostAndPort;
Expand All @@ -10,6 +11,7 @@ public class HostAndPortUtil {
private static List<HostAndPort> redisHostAndPortList = new ArrayList<HostAndPort>();
private static List<HostAndPort> sentinelHostAndPortList = new ArrayList<HostAndPort>();
private static List<HostAndPort> clusterHostAndPortList = new ArrayList<HostAndPort>();
private static List<String> redisUDSList = new ArrayList<String>();

static {
redisHostAndPortList.add(new HostAndPort("localhost", Protocol.DEFAULT_PORT));
Expand All @@ -32,13 +34,17 @@ public class HostAndPortUtil {
clusterHostAndPortList.add(new HostAndPort("localhost", 7383));
clusterHostAndPortList.add(new HostAndPort("localhost", 7384));

redisUDSList.add(new String("/tmp/redis_6379.sock"));

String envRedisHosts = System.getProperty("redis-hosts");
String envSentinelHosts = System.getProperty("sentinel-hosts");
String envClusterHosts = System.getProperty("cluster-hosts");
String envUDSHosts = System.getProperty("uds-hosts");

redisHostAndPortList = parseHosts(envRedisHosts, redisHostAndPortList);
sentinelHostAndPortList = parseHosts(envSentinelHosts, sentinelHostAndPortList);
clusterHostAndPortList = parseHosts(envClusterHosts, clusterHostAndPortList);
redisUDSList = parseUDSHosts(envUDSHosts, redisUDSList);
}

public static List<HostAndPort> parseHosts(String envHosts,
Expand Down Expand Up @@ -76,6 +82,18 @@ public static List<HostAndPort> parseHosts(String envHosts,
return existingHostsAndPorts;
}

public static List<String> parseUDSHosts(String envHosts, List<String> existingUDSHosts) {
if (null != envHosts && 0 < envHosts.length()) {

String[] hostDefs = envHosts.split(",");

if (null != hostDefs) {
return Arrays.asList(hostDefs);
}
}
return existingUDSHosts;
}

public static List<HostAndPort> getRedisServers() {
return redisHostAndPortList;
}
Expand All @@ -87,4 +105,8 @@ public static List<HostAndPort> getSentinelServers() {
public static List<HostAndPort> getClusterServers() {
return clusterHostAndPortList;
}

public static List<String> getUDSServers() {
return redisUDSList;
}
}
15 changes: 15 additions & 0 deletions src/test/java/redis/clients/jedis/tests/UDSTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package redis.clients.jedis.tests;

import org.junit.Test;
import static org.junit.Assert.assertEquals;
import redis.clients.jedis.Jedis;

public class UDSTest {
protected static String udsHost = HostAndPortUtil.getUDSServers().get(0);
@Test
public void testCompareTo() {
Jedis jedis = new Jedis(udsHost);
assertEquals("PONG", jedis.ping());
jedis.close();
}
}