[STORM-1523] util.clj available-port conversion to java

[redsanket]

Please note this is a small change when compared to a bigger util.clj to java PR.
It is being put first as the work needed to convert the entire file is taken care and will be up soon.
Just did not want to depend on other patches so putting this up first.

[abhishekagarwal87]

localPort = getAvailablePort(0);

[abhishekagarwal87]

also this should be done only if the input port is non-zero. similar to what is happening in clojure. since there is no control on recursion depth here.

[redsanket]

Nice catch thanks, I will do that.

[redsanket]

@revans2 @abhishekagarwal87 fixed the issues I presume. Let me know if you have any concerns

[abhishekagarwal87]

will be good to rename the port to preferredPort since it only makes a best effort guarantee.

[redsanket]

@abhishekagarwal87 @knusbaum please take a look at it, it must go into Utils.java

[knusbaum]

+1

[revans2]

+1 lets squash

[redsanket]

@revans squashed

[abhishekagarwal87]

one minor comment. previous version never returns -1. can we just bubble up the exception in catch block if preferredPort is not greater than zero and not return a negative port? Other than that +1.

[redsanket]

@abhishekagarwal87 socket.getLocalPort() returns -1 if no port is found as per the internal implementation. No harm in assigning localPort with -1

/**
* Returns the port number on which this socket is listening.
*

* If the socket was bound prior to being {@link #close closed},
* then this method will continue to return the port number
* after the socket is closed.
*
* @return the port number to which this socket is listening or
* -1 if the socket is not bound yet.
*/
public int getLocalPort() {
if (!isBound())
return -1;
try {
return getImpl().getLocalPort();
} catch (SocketException e) {
// nothing
// If we're bound, the impl has been created
// so we shouldn't get here
}
return -1;
}

[knusbaum]

@abhishekagarwal87 Good catch. I doubt it has any practical effects, but it's good to be consistent.

[knusbaum]

Although, if we want to be consistent with the original code, removing the if and the final return statement is the solution.

[abhishekagarwal87]

If there is an exception, tests are going to fail nonetheless. If the exception is bubbled up, test failure will show the right error. Though yes I agree, that in practice binding to port zero is not likely to result in an exception.

[knusbaum]

Trying with value 0 on failure was a potentially important behavior of the previous implementation.

Exceptions will still bubble up if it fails if my solution is implemented.

[redsanket]

@knusbaum @abhishekagarwal87 The original implementation would return -1 in any case if the port is not bound yet. If we not changing the final outcome and unless we are concerned about throwing an exception when the port is not bound, I guess it would be fine. Moreover, removing if condition would keep trying to find the port recursively until it finds it I presume. The original code only tries once as per my understanding.

[abhishekagarwal87]

Yes. Original code tries only once and if condition should definitely be kept. It's fine with me.

[redsanket]

@revans2 @knusbaum @abhishekagarwal87 still +1 ? Hope it is languishing too long out here.

[revans2]

Yes still +1

[abhishekagarwal87]

+1

[harshach]

@redsanket can you upmerge.

[redsanket]

@harshach upmerged and squashed