Hi Roger,
Your approach is more elegant. However checking for ":0/" may not work
as we can have non-zero port number that can end in 0.
Regards
Harsha
On Tuesday 17 January 2017 09:39 PM, Roger Riggs wrote:
Hi Harsha,
On 1/16/2017 1:21 AM, Harsha Wardhana B wrote:
Hi Amit,
In JdpJmxRemoteDynamicPortTestCase:48 needs null/empty check for jmx
url.
JdpJmxRemoteDynamicPortTestCase:49, array length needs to checked
before accessing index at token[6].
It is possible that port number need not always be present at given
index and hence we may have to follow different approach to extract
port number. Please check if approach below works.
<code>
int idx = jmxurl.indexOf(':');
while (idx != -1) {
jmxurl = jmxurl.substring(idx+1);
idx = jmxurl.indexOf(':');
}
This loop would very eagerly find the last ":" in the string even it
was well past the host/port field.
String.lastIndex would be equivalent.
if(jmxurl.indexOf('/') == -1) {
throw new RuntimeException("Test failed : Invalid
JMXServiceURL");
}
It would be more efficient to compare the index of the '/' after the
last ":" than to re-create new substrings.
int colon = jmxurl.lastIndexOf(':');
int slash = jmxurl.indexOf('/', colon);
int port = Integer.parseInt(jmxurl, colon + 1, slash, 10);
String portStr = jmxurl.substring(0,jmxurl.indexOf('/'));
int port = Integer.parseInt(portStr);
if( port == 0 ) {
throw new RuntimeException("Test failed : Zero port for
jmxremote");
}
Or It might be just as effective to just to check if ":0/" is present.
if (jmxurl.contains(":0/")) {...}
$.02, Roger
</code>
Regards
Harsha
On Monday 16 January 2017 11:16 AM, Amit Sapre wrote:
Thanks Dmitry for the review.
Can I have one more reviewer for this fix ?
Thanks,
Amit
-----Original Message-----
From: Dmitry Samersoff
Sent: Sunday, January 15, 2017 4:49 PM
To: Amit Sapre; serviceability-dev
Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts
"0" instead of assigned port
Amit,
Changes looks good to me.
-Dmitry
On 2017-01-13 09:17, Amit Sapre wrote:
Hello,
Please review the fix for JDK-8167337
Bug Id : https://bugs.openjdk.java.net/browse/JDK-8167337
Webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8167337/webrev.00/
Thanks,
Amit
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the
sources.
