This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new db5923c Clean up the ServerInstance class and remove the un-necessary
ip address stored (#4438)
db5923c is described below
commit db5923cefc60733522a425be07eeb8cd89fdd60f
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Wed Jul 17 10:41:41 2019 -0700
Clean up the ServerInstance class and remove the un-necessary ip address
stored (#4438)
- Remove the un-necessary IP address field
- Remove the error log for the UnknownHostException
- Always use the cached host name and short host name if exists
- This can reduce lots of errors in tests as host passed in might not
exist, e.g. "testHost"
---
.../broker/helix/LiveInstanceChangeHandler.java | 6 +-
.../pinot/common/response/ServerInstance.java | 180 ++++++++-------------
.../pinot/transport/common/ServerInstanceTest.java | 65 +++-----
3 files changed, 86 insertions(+), 165 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/LiveInstanceChangeHandler.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/LiveInstanceChangeHandler.java
index 5162ce4..685a429 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/LiveInstanceChangeHandler.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/LiveInstanceChangeHandler.java
@@ -99,8 +99,7 @@ public class LiveInstanceChangeHandler implements
ClusterChangeHandler {
try {
LOGGER.info("Instance {} has changed session id {} -> {},
validating connection pool for this instance.",
instanceId, sessionId,
_liveInstanceToSessionIdMap.get(instanceId));
- ServerInstance ins = ServerInstance.forHostPort(hostName, port);
- _connectionPool.validatePool(ins, DO_NOT_RECREATE);
+ _connectionPool.validatePool(new ServerInstance(hostName, port),
DO_NOT_RECREATE);
_liveInstanceToSessionIdMap.put(instanceId, sessionId);
} catch (Exception e) {
LOGGER.error("Error trying to validate & destroy dead connections
for {}", instanceId, e);
@@ -111,8 +110,7 @@ public class LiveInstanceChangeHandler implements
ClusterChangeHandler {
// we don't have this instanceId
// lets first check if the connection is valid or not
try {
- ServerInstance ins = ServerInstance.forHostPort(hostName, port);
- _connectionPool.validatePool(ins, DO_NOT_RECREATE);
+ _connectionPool.validatePool(new ServerInstance(hostName, port),
DO_NOT_RECREATE);
_liveInstanceToSessionIdMap.put(instanceId, sessionId);
} catch (Exception e) {
LOGGER.error("Error trying to destroy dead connections for {}",
instanceId, e);
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/response/ServerInstance.java
b/pinot-common/src/main/java/org/apache/pinot/common/response/ServerInstance.java
index effb144..adbe111 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/response/ServerInstance.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/response/ServerInstance.java
@@ -20,55 +20,46 @@ package org.apache.pinot.common.response;
import com.google.common.net.InternetDomainName;
import java.net.InetAddress;
-import java.net.UnknownHostException;
import java.util.concurrent.ConcurrentHashMap;
-import org.apache.commons.lang3.tuple.Triple;
import org.apache.pinot.common.utils.CommonConstants;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import org.apache.pinot.common.utils.EqualityUtils;
/**
* Service abstraction.
- * A service is identified by its hostname and port.
- * Internally, an ip address is also resolved.
- *
- * Nuances:
- * -------
- * A hostname "localhost" will not be resolved to the
- * local hostname and will retain the hostname as "localhost".
- * If the name passed is "127.0.0.1", then it is resolved to the
- * local hostname.
+ * A service is identified by its host, port and sequence id.
*/
-public class ServerInstance implements Comparable<ServerInstance> {
- protected static final Logger LOGGER =
LoggerFactory.getLogger(ServerInstance.class);
-
+public class ServerInstance {
public static final String NAME_PORT_DELIMITER = ":";
public static final String NAME_PORT_DELIMITER_FOR_INSTANCE_NAME = "_";
- /** Host-name where the service is running **/
- private final String _hostname;
+ private static class HostNamePair {
+ String _hostName;
+ String _shortHostName;
- /** Service Port **/
- private final int _port;
+ HostNamePair(String hostName, String shortHostName) {
+ _hostName = hostName;
+ _shortHostName = shortHostName;
+ }
+ }
- /** IP Address. Not used in equals/hash-code generation **/
- private final InetAddress _ipAddress;
+ private static final ConcurrentHashMap<String, HostNamePair> HOST_NAME_MAP =
new ConcurrentHashMap<>();
- private final int _seq;
+ /** Host-name where the service is running **/
+ private final String _hostName;
private final String _shortHostName;
- private static final ConcurrentHashMap<String, Triple<String, String,
InetAddress>> nameToInstanceInfo =
- new ConcurrentHashMap<>();
+ /** Service Port **/
+ private final int _port;
+
+ private final int _seq;
/**
* Use this constructor if the name and port are embedded as string with ":"
as delimiter
- *
- * @param namePortStr Name and Port settings
*/
- public ServerInstance(String namePortStr) {
- this(namePortStr.split(NAME_PORT_DELIMITER)[0],
Integer.parseInt(namePortStr.split(NAME_PORT_DELIMITER)[1]));
+ public ServerInstance(String namePortPair) {
+ this(namePortPair.split(NAME_PORT_DELIMITER)[0],
Integer.parseInt(namePortPair.split(NAME_PORT_DELIMITER)[1]));
}
public ServerInstance(String name, int port) {
@@ -76,31 +67,45 @@ public class ServerInstance implements
Comparable<ServerInstance> {
}
public ServerInstance(String name, int port, int seq) {
- InetAddress ipAddr = null;
-
- try {
- ipAddr = InetAddress.getByName(name);
- } catch (UnknownHostException e) {
- LOGGER.error("Unable to fetch IpAddresses for host:" + name, e);
- ipAddr = null;
- }
-
- _ipAddress = ipAddr;
- _hostname = _ipAddress != null ? _ipAddress.getHostName() : name;
+ HostNamePair hostNamePair = HOST_NAME_MAP.computeIfAbsent(name, key -> {
+ String hostName = getHostName(name);
+ String shortHostName = getShortHostName(hostName);
+ return new HostNamePair(hostName, shortHostName);
+ });
+ _hostName = hostNamePair._hostName;
+ _shortHostName = hostNamePair._shortHostName;
_port = port;
_seq = seq;
- _shortHostName = makeShortHostName(_hostname);
}
- private ServerInstance(String hostname, String shortHostName, InetAddress
ipAddress, int port, int seq) {
- _hostname = hostname;
+ private ServerInstance(String hostName, String shortHostName, int port, int
seq) {
+ _hostName = hostName;
_shortHostName = shortHostName;
- _ipAddress = ipAddress;
_port = port;
_seq = seq;
}
/**
+ * Server instance name is formatted as {@code Server_<host>_<port>}
+ */
+ public static ServerInstance forInstanceName(String instanceName) {
+ String[] parts =
instanceName.split(CommonConstants.Helix.PREFIX_OF_SERVER_INSTANCE)[1]
+ .split(NAME_PORT_DELIMITER_FOR_INSTANCE_NAME);
+ return new ServerInstance(parts[0], Integer.parseInt(parts[1]));
+ }
+
+ /**
+ * Name can either be a machine name, or a textual representation of its IP
address.
+ */
+ private static String getHostName(String name) {
+ try {
+ return InetAddress.getByName(name).getHostName();
+ } catch (Exception e) {
+ return name;
+ }
+ }
+
+ /**
* As per <a href="https://tools.ietf.org/html/rfc952";>RFC-952</a> domain
names should begin with a letter.
* That said, <a
href="https://tools.ietf.org/html/rfc1123#page-13";>RFC-1123</a> updated it say
that it may also begin
* with a digit. Indeed, <a href="http://9292.nl/";>this</a> is a valid
domain name. Only the top-level domain (i.e. the
@@ -119,39 +124,33 @@ public class ServerInstance implements
Comparable<ServerInstance> {
*
* It will fail if there are host names starting with a digit, but will work
right otherwise.
*/
- private String makeShortHostName(final String name) {
+ private static String getShortHostName(String hostName) {
try {
- InternetDomainName domainName = InternetDomainName.from(name);
- return domainName.parts().get(0);
+ return InternetDomainName.from(hostName).parts().get(0);
} catch (Exception e) {
- return name;
+ return hostName;
}
}
- public String getShortHostName() {
- return _shortHostName;
+ public String getHostname() {
+ return _hostName;
}
- public String getHostname() {
- return _hostname;
+ public String getShortHostName() {
+ return _shortHostName;
}
public int getPort() {
return _port;
}
- public InetAddress getIpAddress() {
- return _ipAddress;
+ public ServerInstance withSeq(int seq) {
+ return new ServerInstance(_hostName, _shortHostName, _port, seq);
}
@Override
public int hashCode() {
- final int prime = 31;
- int result = 1;
- result = (prime * result) + (_hostname == null ? 0 : _hostname.hashCode());
- result = (prime * result) + _port;
- result = (prime * result) + _seq;
- return result;
+ return
EqualityUtils.hashCodeOf(EqualityUtils.hashCodeOf(_hostName.hashCode(), _port),
_seq);
}
@Override
@@ -159,66 +158,15 @@ public class ServerInstance implements
Comparable<ServerInstance> {
if (this == obj) {
return true;
}
- if (obj == null) {
- return false;
- }
- if (getClass() != obj.getClass()) {
- return false;
- }
- ServerInstance other = (ServerInstance) obj;
- if (_hostname == null) {
- if (other._hostname != null) {
- return false;
- }
- } else if (!_hostname.equals(other._hostname)) {
- return false;
- }
- if (_port != other._port) {
- return false;
+ if (obj instanceof ServerInstance) {
+ ServerInstance that = (ServerInstance) obj;
+ return _hostName.equals(that._hostName) && _port == that._port && _seq
== that._seq;
}
- if (_seq != other._seq) {
- return false;
- }
- return true;
- }
-
- public ServerInstance withSeq(int seq) {
- return new ServerInstance(_hostname, _shortHostName, _ipAddress, _port,
seq);
+ return false;
}
@Override
public String toString() {
- return _hostname + "_" + _port;
- }
-
- @Override
- public int compareTo(ServerInstance o) {
- return this.toString().compareTo(o.toString());
- }
-
- public static ServerInstance forHostPort(String name, int port) {
- if (nameToInstanceInfo.containsKey(name)) {
- Triple<String, String, InetAddress> instanceInfo =
nameToInstanceInfo.get(name);
- return new ServerInstance(instanceInfo.getLeft(),
instanceInfo.getMiddle(), instanceInfo.getRight(), port, 0);
- } else {
- ServerInstance newInstance = new ServerInstance(name, port);
-
- nameToInstanceInfo.putIfAbsent(name,
- Triple.of(newInstance.getHostname(), newInstance.getShortHostName(),
newInstance.getIpAddress()));
-
- return newInstance;
- }
- }
-
- public static ServerInstance forInstanceName(String instanceName) {
- String namePortStr =
instanceName.split(CommonConstants.Helix.PREFIX_OF_SERVER_INSTANCE)[1];
- String hostName =
namePortStr.split(NAME_PORT_DELIMITER_FOR_INSTANCE_NAME)[0];
- int port;
- try {
- port =
Integer.parseInt(namePortStr.split(NAME_PORT_DELIMITER_FOR_INSTANCE_NAME)[1]);
- } catch (Exception e) {
- port = CommonConstants.Helix.DEFAULT_SERVER_NETTY_PORT;
- }
- return forHostPort(hostName, port);
+ return _hostName + "_" + _port;
}
}
diff --git
a/pinot-transport/src/test/java/org/apache/pinot/transport/common/ServerInstanceTest.java
b/pinot-transport/src/test/java/org/apache/pinot/transport/common/ServerInstanceTest.java
index f04f877..5c89d25 100644
---
a/pinot-transport/src/test/java/org/apache/pinot/transport/common/ServerInstanceTest.java
+++
b/pinot-transport/src/test/java/org/apache/pinot/transport/common/ServerInstanceTest.java
@@ -18,61 +18,36 @@
*/
package org.apache.pinot.transport.common;
-import java.net.InetAddress;
import org.apache.pinot.common.response.ServerInstance;
-import org.testng.Assert;
import org.testng.annotations.Test;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+
public class ServerInstanceTest {
@Test
- public void testServerInstance()
- throws Exception {
- // Same local hostname and port
- {
- ServerInstance instance1 = new ServerInstance("localhost", 8080);
- ServerInstance instance2 = new ServerInstance("localhost", 8080);
-// System.out.println("Instance 1 :" + instance1);
-// System.out.println("Instance 2 :" + instance2);
- Assert.assertEquals(instance2, instance1, "Localhost server-instances
with same port");
- }
+ public void testServerInstance() {
+ // Same local host name and port
+ assertEquals(new ServerInstance("localhost", 8080), new
ServerInstance("localhost", 8080));
+
+ // Same IP address host and port
+ assertEquals(new ServerInstance("127.0.0.1", 8080), new
ServerInstance("127.0.0.1", 8080));
+
+ // Same other host name and port
+ assertEquals(new ServerInstance("test-host", 8080), new
ServerInstance("test-host", 8080));
- // Same hostname and port
- {
- ServerInstance instance1 = new ServerInstance("test-host", 8080);
- ServerInstance instance2 = new ServerInstance("test-host", 8080);
-// System.out.println("Instance 1 :" + instance1);
-// System.out.println("Instance 2 :" + instance2);
- Assert.assertEquals(instance2, instance1, "Localhost server-instances
with same port");
- }
- // same hostname but different port
- {
- ServerInstance instance1 = new ServerInstance("localhost", 8081);
- ServerInstance instance2 = new ServerInstance("localhost", 8082);
-// System.out.println("Instance 1 :" + instance1);
-// System.out.println("Instance 2 :" + instance2);
- Assert.assertFalse(instance1.equals(instance2), "Localhost
server-instances with same port");
- }
+ // Same other IP address host and port
+ assertEquals(new ServerInstance("192.168.0.1", 8080), new
ServerInstance("192.168.0.1", 8080));
- // same port but different host
- {
- ServerInstance instance1 = new ServerInstance("abcd", 8080);
- ServerInstance instance2 = new ServerInstance("abce", 8080);
-// System.out.println("Instance 1 :" + instance1);
-// System.out.println("Instance 2 :" + instance2);
- Assert.assertFalse(instance1.equals(instance2), "Localhost
server-instances with same port");
- }
+ // Same local host and port, one with host name and one with IP address
+ assertEquals(new ServerInstance("localhost", 8080), new
ServerInstance("127.0.0.1", 8080));
- // Test getIpAddress
- {
- InetAddress ipAddr = InetAddress.getByName("127.0.0.1");
+ // Same host but different port
+ assertNotEquals(new ServerInstance("localhost", 8081), new
ServerInstance("localhost", 8082));
- ServerInstance instance1 = new ServerInstance("127.0.0.1", 8080);
- Assert.assertEquals(instance1.getPort(), 8080, "Port check");
- Assert.assertEquals(instance1.getPort(), 8080, "Hostname check");
- Assert.assertEquals(instance1.getHostname(), ipAddr.getHostName(), "Host
check");
- Assert.assertEquals(instance1.getIpAddress(), ipAddr, "IP check");
- }
+ // Same port but different host
+ assertNotEquals(new ServerInstance("foo", 8080), new ServerInstance("bar",
8080));
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
