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()```
--
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]
