markrmiller commented on a change in pull request #1659:
URL: https://github.com/apache/hbase/pull/1659#discussion_r420466741
##########
File path:
hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseCommonTestingUtility.java
##########
@@ -264,4 +265,34 @@ boolean deleteDir(final File dir) {
boolean failIfTimeout, Predicate<E> predicate) throws E {
return Waiter.waitFor(this.conf, timeout, interval, failIfTimeout,
predicate);
}
+
+ private static final PortAllocator portAllocator = new PortAllocator();
+
+ public static int randomFreePort() {
+ try {
+ return portAllocator.randomFreePort();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ static class PortAllocator {
+
+ public PortAllocator() {
+
+ }
+
+ public int randomFreePort() throws IOException {
+ ServerSocket s = new ServerSocket(0);
+ try {
+ s.setReuseAddress(true);
+ int port = s.getLocalPort();
+ return port;
+ } finally {
+ if (null != s) {
+ s.close();
+ }
+ }
+ }
+ }
}
Review comment:
Yeah, I'm not too concerned how that test itself is addressed, but I
think it's good to cleanup getting the port. There are multiple implementations
and the one currently used in this test is weak and more complicated vs the
other of just using port 0 (and properly using setReuseAddrress=true).
You have to also still catch a bind exception because there is a race - it's
better to specify 0 for the socket that will actually be used, not see that a
port is free and then try to use it (may not be free a second later).
Your solution is isolated to that test though right? The tests that use
these other 2 get port methods have the same race, so it seems better to
combine the get free port methods into one and make that robust?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
