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

[Roger Riggs]

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; 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170119/68d01911/attachment.html>