JDK-8031753 : Review request

[Jaroslav Bachorik]

Hi Harsha,

On 27.1.2016 15:30, Harsha Wardhana B wrote:
> Hi Jaroslav,
>
> Below is the updated webrev with minor line alignment fixes.
>
> http://cr.openjdk.java.net/~hb/8031753/webrev.01/
> <http://cr.openjdk.java.net/%7Ehb/8031753/webrev.01/>
>
> The issue can be reproduced by removing entry for hostname in
> /etc/hosts (for linux) and running
> 'javax/management/remote/mandatory/connection/RMIConnectionIdTest'
> test.
> Hence no new jtreg test case have been written.
>
> Agreed that we are putting involved logic into a container object. The
> alternative would be remove all the involved logic from JMXServiceURL
> and create new a helper classes that can do URL validation. But that
> would be a API/Design change that can break compatibility with existing
> applications. Since the bug is not critical, I think we can make do with
> adding little more involved logic into existing validations.

I see. Probably this is acceptable.

>
> I would welcome more comments and discussions around this change. Please
> do review and let me know your thoughts.

(R)eviewed.

Thanks!

-JB-

>
> Thanks
> Harsha
>
> On Wednesday 27 January 2016 03:47 PM, Jaroslav Bachorik wrote:
>> Hi Harsha,
>>
>> On 27.1.2016 11:09, Harsha Wardhana B wrote:
>>> Hello All,
>>>
>>> For the below fix, I have made appropriate javadoc changes and uploaded
>>> new webrev at below location.
>>>
>>> http://cr.openjdk.java.net/~hb/8031753/webrev.00/
>>> <http://cr.openjdk.java.net/%7Ehb/8031753/webrev.00/>
>>>
>>> Please review and let me know your comments.
>>
>> I'm a bit uneasy by introducing rather involved logic into supposedly
>> simple value holder type JMXServiceURL. On the other hand, there
>> already are some pieces of non-trivial code to deduce defaults there
>> so we might just continue on this path.
>>
>> Here are some nits:
>>
>> src/java.management/share/classes/javax/management/remote/JMXServiceURL.java
>>
>> L309-319: Indentation seem to be corrupted here
>> L365-366: The opening curly bracket should be on the same line
>>
>> Missing reg test.
>>
>> Cheers,
>>
>> -JB-
>>
>>>
>>> Thanks
>>> Harsha
>>>
>>> On Thursday 14 January 2016 09:34 AM, Harsha Wardhana B wrote:
>>>> Gentle reminder :)
>>>>
>>>> On Monday 11 January 2016 02:51 PM, Jaroslav Bachorik wrote:
>>>>> [adding JMX dev list]
>>>>>
>>>>> On 8.1.2016 10:44, Harsha Wardhana B wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Please review the fix for,
>>>>>>
>>>>>> Issue : JDK-8031753 <http://JDK-8031753> - JMXServiceURL should
>>>>>> not use
>>>>>> getLocalHost or its usage should be enhanced
>>>>>> Webrev :
>>>>>> http://cr.openjdk.java.net/~jbachorik/sponsorship/8031753/webrev.00/
>>>>>>
>>>>>> The issue can be reproduced by removing entry for hostname in
>>>>>> /etc/hosts
>>>>>> (for linux) and running
>>>>>> 'javax/management/remote/mandatory/connection/RMIConnectionIdTest'
>>>>>> test.
>>>>>> Hence no new jtreg test case have been written.
>>>>>>
>>>>>> Fix Description:
>>>>>>
>>>>>> If JMXServiceURL is given null host, it tries to resolve hostname via
>>>>>> InetAddress.getLocalHost() and if that fails throws
>>>>>> MalformedURLException. Since JMX Agent will listen on all interfaces
>>>>>> (0.0.0.0) if host is null, the fix handles the exception and assigns
>>>>>> first active non-loopback interface IP address to host. That way
>>>>>> we can
>>>>>> have JMXServiceURL with IP address instead of hostname.
>>>>>>
>>>>>> It is possible that we might end up constructing JMXServiceURL
>>>>>> with IP
>>>>>> for an interface which clients may not be able to connect (because of
>>>>>> network reachability issues). In that case, the recommendation
>>>>>> would be
>>>>>> to try to start JMX Agent with
>>>>>> com.sun.management.jmxremote.host=xx.xx.xx.xx option. That way, the
>>>>>> agent will be started on interface that clients can actually connect
>>>>>> to.
>>>>>>
>>>>>> Please review the fix and let me know your comments. The fix requires
>>>>>> amending javadoc with above behavioral change and a CCC review as
>>>>>> well
>>>>>> which I will send out later.
>>>>>>
>>>>>> Thanks
>>>>>> Harsha
>>>>>>
>>>>>
>>>>
>>>
>>
>