saintstack commented on code in PR #4692:
URL: https://github.com/apache/hadoop/pull/4692#discussion_r949336930
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java:
##########
@@ -1753,7 +1756,28 @@ public ConnectionId(InetSocketAddress address, Class<?>
protocol,
InetSocketAddress getAddress() {
return address;
}
-
+
+ /**
+ * This is used to update the remote address when an address change is
detected. This method
+ * ensures that the {@link #hashCode()} won't change.
+ *
+ * @param address the updated address
+ * @throws IllegalArgumentException if the hostname or port doesn't match
+ * @see Connection#updateAddress()
Review Comment:
The IllegalAddressException throw is unsettling but makes sense as long as
the caller #updateAddress keeps making the currentAddr in the same way... I was
going to say this method comment could be more explicit about what is happening
in here and presumptions but I think the pointer to #updateAddress takes care
of it....
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java:
##########
@@ -815,6 +817,81 @@ public Void call() throws IOException {
}
}
+ /**
+ * The {@link ConnectionId#hashCode} has to be stable despite updates that
occur as the the
+ * address evolves over time. The {@link ConnectionId} is used as a primary
key in maps, so
+ * its hashCode can't change.
+ *
+ * @throws IOException if there is a client or server failure
+ */
+ @Test
+ public void testStableHashCode() throws IOException {
+ Server server = new TestServer(5, false);
+ try {
+ server.start();
+
+ // Leave host unresolved to start. Use "localhost" as opposed
+ // to local IP from NetUtils.getConnectAddress(server) to force
+ // resolution later
+ InetSocketAddress unresolvedAddr = InetSocketAddress.createUnresolved(
+ "localhost", NetUtils.getConnectAddress(server).getPort());
+
+ // Setup: Create a ConnectionID using an unresolved address, and get
it's hashCode to serve
+ // as a point of comparison.
+ int rpcTimeout = MIN_SLEEP_TIME * 2;
+ final ConnectionId remoteId = getConnectionId(unresolvedAddr,
rpcTimeout, conf);
+ int expected = remoteId.hashCode();
+
+ // Start client
+ Client.setConnectTimeout(conf, 100);
+ Client client = new Client(LongWritable.class, conf);
+ try {
+ // Test: Call should re-resolve host and succeed
+ LongWritable param = new LongWritable(RANDOM.nextLong());
+ client.call(RPC.RpcKind.RPC_BUILTIN, param, remoteId,
+ RPC.RPC_SERVICE_CLASS_DEFAULT, null);
+ int actual = remoteId.hashCode();
+
+ // Verify: The hashCode should match, although the InetAddress is
different since it has
+ // now been resolved
+ assertThat(remoteId.getAddress()).isNotEqualTo(unresolvedAddr);
+
assertThat(remoteId.getAddress().getHostName()).isEqualTo(unresolvedAddr.getHostName());
+ assertThat(remoteId.hashCode()).isEqualTo(expected);
+
+ // Test: Call should succeed without having to re-resolve
+ InetSocketAddress expectedSocketAddress = remoteId.getAddress();
+ param = new LongWritable(RANDOM.nextLong());
+ client.call(RPC.RpcKind.RPC_BUILTIN, param, remoteId,
+ RPC.RPC_SERVICE_CLASS_DEFAULT, null);
+
+ // Verify: The same instance of the InetSocketAddress has been used to
make the second
+ // call
+ assertThat(remoteId.getAddress()).isSameAs(expectedSocketAddress);
+
+ // Verify: The hashCode is protected against updates to the host name
+ String hostName = InetAddress.getLocalHost().getHostName();
+ InetSocketAddress mismatchedHostName = NetUtils.createSocketAddr(
+ InetAddress.getLocalHost().getHostName(),
+ remoteId.getAddress().getPort());
+ assertThatExceptionOfType(IllegalArgumentException.class)
+ .isThrownBy(() -> remoteId.setAddress(mismatchedHostName))
+ .withMessageStartingWith("Hostname must match");
+
+ // Verify: The hashCode is protected against updates to the port
+ InetSocketAddress mismatchedPort = NetUtils.createSocketAddr(
+ remoteId.getAddress().getHostName(),
+ remoteId.getAddress().getPort() + 1);
+ assertThatExceptionOfType(IllegalArgumentException.class)
+ .isThrownBy(() -> remoteId.setAddress(mismatchedPort))
+ .withMessageStartingWith("Port must match");
+ } finally {
+ client.stop();
+ }
+ } finally {
+ server.stop();
+ }
+ }
+
Review Comment:
Nice test.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java:
##########
@@ -323,7 +323,7 @@ public <T> ProtocolProxy<T> getProxy(Class<T> protocol,
long clientVersion,
Client.ConnectionId connId, Configuration conf, SocketFactory factory)
throws IOException {
return getProxy(protocol, clientVersion, connId.getAddress(),
- connId.ticket, conf, factory, connId.getRpcTimeout(),
+ connId.getTicket(), conf, factory, connId.getRpcTimeout(),
Review Comment:
Nice but unrelated change.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java:
##########
@@ -815,6 +817,81 @@ public Void call() throws IOException {
}
}
+ /**
+ * The {@link ConnectionId#hashCode} has to be stable despite updates that
occur as the the
Review Comment:
Double 'the'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
