[
https://issues.apache.org/jira/browse/HADOOP-18365?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17581419#comment-17581419
]
ASF GitHub Bot commented on HADOOP-18365:
-----------------------------------------
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'.
> Updated addresses are still accessed using the old IP address
> -------------------------------------------------------------
>
> Key: HADOOP-18365
> URL: https://issues.apache.org/jira/browse/HADOOP-18365
> Project: Hadoop Common
> Issue Type: Improvement
> Components: common
> Environment: Demonstrated in a Kubernetes environment running Java 11.
> Reporter: Steve Vaughan
> Assignee: Steve Vaughan
> Priority: Major
> Labels: pull-request-available
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> When the IPC Client recognizes that an IP address has changed, it updates the
> server field and logs a message:
> Address change detected. Old:
> journalnode-1.journalnode.hdfs.svc.cluster.local/10.1.0.178:8485 New:
> journalnode-1.journalnode.hdfs.svc.cluster.local/10.1.0.182:8485
> Although the change is detected, the client will continue to connect to the
> old IP address, resulting in repeated log messages. This is seen in managed
> environments when JournalNode syncing is enabled and a JournalNode is
> restarted, with the remaining nodes in the set repeatedly logging this
> message when syncing to the restarted JournalNode.
> The source of the problem is that the remoteId.address is not updated.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
