jmx-dev RFR(S): 8234484: Add ability to configure third port for remote JMX

[Daniel Fuchs]

Hi Felix,

On 11/01/2020 07:37, Yangfei (Felix) wrote:
> Hi Daniel,
> 
>      Thanks for the suggestions.
>      I modified the patch making the third port also configurable via the management.properties file.
>      New webrev: http://cr.openjdk.java.net/~fyang/8234484/webrev.01/

I think I would prefer the try catch NumberFormatException
to be just around the call to Integer.parseInt(), as it is
done elsewhere in this file (e.g. line 337).

> 
>      Previously, we test the effectiveness of the patch by checking the port usage with -Dsun.rmi.transport.tcp.logLevel=VERBOSE.
>      Is it good to modify the following test case specifying the third port for the testing purpose?

Please don't do that. That test is not appropriate for this - as
it opens a remote connector, not a local connector.

Also please get someone from serviceability-dev to also look at
changes (added in cc:).

best regards,

-- daniel

> 
> diff -r d630c0a63222 test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> --- a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java	Wed Jan 08 08:53:28 2020 +0900
> +++ b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java	Sat Jan 11 15:30:25 2020 +0000
> @@ -31,6 +31,7 @@
>   
>   import jdk.test.lib.process.OutputAnalyzer;
>   import jdk.test.lib.process.ProcessTools;
> +import jdk.test.lib.Utils;
>   
>   /**
>    * @test
> @@ -203,6 +204,12 @@
>               args.add("-Dcom.sun.management.jmxremote.host=" + address);
>               args.add("-Dcom.sun.management.jmxremote.port=" + jmxPort);
>               args.add("-Dcom.sun.management.jmxremote.rmi.port=" + rmiPort);
> +            try {
> +                int port = Utils.getFreePort();
> +                args.add("-Dcom.sun.management.jmxlocal.port=" + port);
> +            } catch (Exception e) {
> +                System.out.println(e);
> +            }
>               args.add("-Dcom.sun.management.jmxremote.authenticate=false");
>               args.add("-Dcom.sun.management.jmxremote.ssl=" + Boolean.toString(useSSL));
>               // This is needed for testing on loopback
> 
> Felix
> 
>>
>> Hi Felix,
>>
>> Shouldn't this be configurable via the management.properties file, like all other
>> JMX properties?
>>
>> Also on the one hand - it would be good to have a test.
>> On the other hand - writing a stable test will be problematic as there is no way
>> to ensure that any particular port number will be available for the agent to bind
>> to.
>>
>> best regards,
>>
>> -- daniel
>>
>> On 08/01/2020 01:09, Yangfei (Felix) wrote:
>>> Hi,
>>>
>>> Please review this patch adding a way to configure the third port for
>>> remote JMX.
>>>
>>> Currently, it is possible to configure two of the three ports for JMX
>>> with com.sun.management.jmxremote.port and
>>> com.sun.management.jmxremote.rmi.port.
>>>
>>> However, there is no way to configure the third port. This can cause
>>> problems if the randomly assigned port conflicts with another service
>>> that has not yet acquired its port.
>>>
>>>      Bug: https://bugs.openjdk.java.net/browse/JDK-8234484
>>>
>>>     Webrev: http://cr.openjdk.java.net/~fyang/8234484/webrev.00/
>>>
>>>     Passed tiered-1 test.
>>>
>>> Thanks,
>>>
>>> Felix
>>>
>