[ https://issues.apache.org/jira/browse/HADOOP-18694?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17714730#comment-17714730 ]
ASF GitHub Bot commented on HADOOP-18694: ----------------------------------------- ayushtkn commented on code in PR #5542: URL: https://github.com/apache/hadoop/pull/5542#discussion_r1173022015 ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java: ########## @@ -590,7 +590,7 @@ private synchronized boolean updateAddress() throws IOException { InetSocketAddress currentAddr = NetUtils.createSocketAddrForHost( server.getHostName(), server.getPort()); - if (!server.equals(currentAddr)) { + if (!currentAddr.isUnresolved() && !server.equals(currentAddr)) { LOG.warn("Address change detected. Old: " + server.toString() + " New: " + currentAddr.toString()); Review Comment: Since you are touching this part can you fix this log line as well? ``` LOG.warn("Address change detected. Old: {} New: {}", server, currentAddr); ``` ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java: ########## @@ -1728,6 +1728,55 @@ public void testProxyUserBinding() throws Exception { checkUserBinding(true); } + @Test(timeout=60000) + public void testUpdateAddressEnsureResolved() throws Exception { + // start server + Server server = new TestServer(5, false); + server.start(); + + SocketFactory mockFactory = Mockito.mock(SocketFactory.class); + doThrow(new ConnectTimeoutException("fake")).when(mockFactory).createSocket(); + Client client = new Client(LongWritable.class, conf, mockFactory); + InetSocketAddress address = new InetSocketAddress("localhost", 9090); + ConnectionId remoteId = getConnectionId(address, 100, conf); + try { + LambdaTestUtils.intercept(IOException.class, + new Callable<Void>() { + @Override + public Void call() throws IOException { + client.call(RpcKind.RPC_BUILTIN, + new LongWritable(RANDOM.nextLong()), remoteId, + RPC.RPC_SERVICE_CLASS_DEFAULT, null); + return null; + } + }); + + assertFalse(address.isUnresolved()); + assertFalse(remoteId.getAddress().isUnresolved()); + assertEquals(System.identityHashCode(remoteId.getAddress()), + System.identityHashCode(address)); + + NetUtils.addStaticResolution("localhost", "host.invalid"); + LambdaTestUtils.intercept(IOException.class, + new Callable<Void>() { + @Override + public Void call() throws IOException { + client.call(RpcKind.RPC_BUILTIN, + new LongWritable(RANDOM.nextLong()), remoteId, + RPC.RPC_SERVICE_CLASS_DEFAULT, null); + return null; + } + }); Review Comment: Use lambda ``` LambdaTestUtils.intercept(IOException.class, (Callable<Void>) () -> { client.call(RpcKind.RPC_BUILTIN, new LongWritable(RANDOM.nextLong()), remoteId, RPC.RPC_SERVICE_CLASS_DEFAULT, null); return null; }); ``` ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java: ########## @@ -1728,6 +1728,55 @@ public void testProxyUserBinding() throws Exception { checkUserBinding(true); } + @Test(timeout=60000) + public void testUpdateAddressEnsureResolved() throws Exception { + // start server + Server server = new TestServer(5, false); + server.start(); + + SocketFactory mockFactory = Mockito.mock(SocketFactory.class); + doThrow(new ConnectTimeoutException("fake")).when(mockFactory).createSocket(); + Client client = new Client(LongWritable.class, conf, mockFactory); + InetSocketAddress address = new InetSocketAddress("localhost", 9090); + ConnectionId remoteId = getConnectionId(address, 100, conf); + try { + LambdaTestUtils.intercept(IOException.class, + new Callable<Void>() { + @Override + public Void call() throws IOException { + client.call(RpcKind.RPC_BUILTIN, + new LongWritable(RANDOM.nextLong()), remoteId, + RPC.RPC_SERVICE_CLASS_DEFAULT, null); + return null; + } + }); Review Comment: Use lambda ``` LambdaTestUtils.intercept(IOException.class, (Callable<Void>) () -> { client.call(RpcKind.RPC_BUILTIN, new LongWritable(RANDOM.nextLong()), remoteId, RPC.RPC_SERVICE_CLASS_DEFAULT, null); return null; }); ``` ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java: ########## @@ -1728,6 +1728,55 @@ public void testProxyUserBinding() throws Exception { checkUserBinding(true); } + @Test(timeout=60000) + public void testUpdateAddressEnsureResolved() throws Exception { + // start server + Server server = new TestServer(5, false); Review Comment: Why do you need 5, 1 should work as well? ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java: ########## @@ -1728,6 +1728,55 @@ public void testProxyUserBinding() throws Exception { checkUserBinding(true); } + @Test(timeout=60000) + public void testUpdateAddressEnsureResolved() throws Exception { + // start server + Server server = new TestServer(5, false); + server.start(); + + SocketFactory mockFactory = Mockito.mock(SocketFactory.class); + doThrow(new ConnectTimeoutException("fake")).when(mockFactory).createSocket(); + Client client = new Client(LongWritable.class, conf, mockFactory); + InetSocketAddress address = new InetSocketAddress("localhost", 9090); Review Comment: Does 9090 has any relevance here? If the port is occupied in the env where test run, will there be any impact? If yes, then use ```NetUtils.getFreeSocketPort()``` > Client.Connection#updateAddress needs to ensure that address is resolved > before updating > ---------------------------------------------------------------------------------------- > > Key: HADOOP-18694 > URL: https://issues.apache.org/jira/browse/HADOOP-18694 > Project: Hadoop Common > Issue Type: Improvement > Components: common > Affects Versions: 3.4.0, 3.3.5 > Reporter: dzcxzl > Priority: Major > Labels: pull-request-available > > When Client.Connection#setupConnection encounters an IOException, it will try > to update the server address. > ([HADOOP-18365|https://issues.apache.org/jira/browse/HADOOP-18365]) > When the address is re-parsed, it may be an unresolved address > (UnknownHostException), which causes Client.Connection#setupConnection to > fail to reconnect. > {code:java} > while (true) { > try { > if (server.isUnresolved()) { {code} > Especially when DN is connected to NN, BPServiceActor#bpNamenode is only > initialized once, which causes DN to never connect to NN before restarting. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org