Copilot commented on code in PR #8472:
URL: https://github.com/apache/hadoop/pull/8472#discussion_r3197964550
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java:
##########
@@ -22,6 +22,8 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.IOException;
Review Comment:
`java.io.IOException` is imported but not used in this test class. This will
fail the build under the project Checkstyle rules (`UnusedImports`). Remove the
unused import (or use it if intended).
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java:
##########
@@ -235,19 +237,45 @@ private static int startRpcServer(boolean
allowInsecurePorts)
"localhost", serverPort, 100000, 1, 2, allowInsecurePorts);
tcpServer = new SimpleTcpServer(serverPort, program, 1);
tcpServer.run();
- break; // Successfully bound a port, break out.
- } catch (InterruptedException | ChannelException e) {
+ return serverPort; // Successfully bound a port.
+ } catch (InterruptedException e) {
if (tcpServer != null) {
tcpServer.shutdown();
}
- if (retries-- > 0) {
- serverPort += rand.nextInt(20); // Port in use? Try another.
- } else {
- throw e; // Out of retries.
+ Thread.currentThread().interrupt();
+ throw e;
+ } catch (Exception e) {
+ // Netty's ChannelFuture#sync may "sneaky-throw" a checked exception
+ // such as java.net.BindException via PlatformDependent#throwException,
+ // which is why catching ChannelException alone is not sufficient.
+ // Only retry on port-in-use style failures; rethrow anything else.
+ if (tcpServer != null) {
+ tcpServer.shutdown();
}
+ if (!isPortInUse(e) || retries-- <= 0) {
+ throw e; // Not a port-in-use error, or out of retries.
+ }
+ // Port in use? Try another. Ensure we never re-pick the same port.
+ serverPort += 1 + rand.nextInt(20);
+ }
+ }
+ }
+
+ /**
+ * Check whether the given exception indicates that the port is already
+ * bound by another process. Handles {@link ChannelException} thrown by
+ * Netty as well as {@link BindException} that may be sneaky-thrown from
+ * {@code ChannelFuture#sync()}.
+ */
+ private static boolean isPortInUse(Throwable t) {
+ Throwable cursor = t;
+ while (cursor != null) {
+ if (cursor instanceof BindException || cursor instanceof
ChannelException) {
+ return true;
Review Comment:
`isPortInUse` currently treats any `ChannelException` as a port-in-use
condition (`cursor instanceof ChannelException`). Netty uses `ChannelException`
for many bind failures (e.g., permission denied, invalid address), so this can
cause retries that mask real errors and make failures harder to diagnose.
Consider only retrying when a `BindException` is present in the cause chain
(and optionally verifying the message is the address-in-use case), rather than
matching `ChannelException` unconditionally.
--
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]
