RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts "0" instead of assigned port

[Amit Sapre]

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; serviceability-dev at 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: serviceability-dev at 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.
> > >>>
> > >>
> > >
> >