Thanks for the review Roger !
From: Roger Riggs Sent: Friday, January 20, 2017 1:49 AM To: Amit Sapre; serviceability-dev@openjdk.java.net Cc: Harsha Wardhana B; Dmitry Samersoff Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port Hi Amit, Looks fine to me. Thanks, Roger On 1/19/2017 6:54 AM, Amit Sapre wrote: Hi, I updated the parsing logic for extracting port number in test case. Here is the updated webrev : http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8167337/webrev.01/ Thanks, Amit -----Original Message----- From: Amit Sapre Sent: Wednesday, January 18, 2017 2:09 PM To: Roger Riggs; HYPERLINK "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net; Harsha Wardhana B Subject: RE: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port Hello, Looks like basic check on Jdp packet includes a check for JMX_SERVICE_URL https://java.se.oracle.com/source/xref/jdk9- dev/jdk/test/sun/management/jdp/JdpTestCase.java#184 I feel we don't need any further check on jmx service url. -----Original Message----- From: Roger Riggs Sent: Tuesday, January 17, 2017 10:05 PM To: HYPERLINK "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port Hi, yes, but the pattern looks for the ":" before and the "/" after the zero. It would not match the port ":000000/" ; in this test code the URL is assumed/known to be relatively well formed. Roger I kept the focus on what needs to be tested. This however doesn't mean we shouldn't validate the url format. I would prefer to do that in a separate test case all together. On 1/17/2017 11:26 AM, Harsha Wardhana B wrote: 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.
