JDK-8031753 : Review request

[Harsha Wardhana B]

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 would welcome more comments and discussions around this change. Please 
do review and let me know your thoughts.

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