RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

[mandy chung]

On 4/27/18 3:43 PM, Harsha Wardhana B wrote:
>
>
> On Thursday 26 April 2018 09:09 PM, mandy chung wrote:
>> src/java.base/share/classes/sun/launcher/resources/launcher.properties
>> 112 \ --start-management-agent option=value[:option=value:....]\n\
>>
>> option and value should be <option> and <value> to represent user-supplied name/value.
> The syntax used is borrowed from Java tool guide,
>
> https://docs.oracle.com/javase/10/tools/java.htm#JSWOR624

I checked java -h output.  <...> is the format for variables.
>
> It is also part of java -help output which has been already approved 
> in the CSR.

This is minor editing.   Most importantly the output of java -help 
should be consistent.

>>   113 \ start the default management agent with semi-colon separated\n\
>>   114 \ list of options. Multiple values for an option should be 
>> separated\n\
>> 115 \ by comma. See the java tool guide for a list of options for\n\
>> 116 \ the default management agent.\n\ typo: "semi-colon separated 
>> list" should be colon-separated list "See the java tool guide...." - 
>> should be "See the Java Monitoring and Management Guide for details"
> I will fix the typo. But the extended help for java is part of the 
> tool guide and not part of management guide. Hence I would like to 
> keep the original reference to tool guide.

Are you proposing to document all options for management properties in 
java tool guide?  Details about --start-management-agent belongs to the 
management guide (not java tool guide).  There are several options 
listed in the java -help output belong to other component for 
-javaagent, -splash, etc.
>>      
>>   719 System.setProperty((String) a, (String) b);I don't expect 
>> --start-management-agent will set the system properties like if 
>> -Dcom.sun.management.config.file=config.file which will not set 
>> additional system properties, right? It's also not specified in CSR. I see the existing code calling System.getProperty is not modified.  I think
>> that may need to be updated too?
> I did not understand. Can you please elaborate.

When --start-management-agent:port=1234 is set, I don't expect the 
system property com.sun.management.jmxremote.port will be set but your 
current implementation does that.   I expect it be consistent with 
-Dcom.sun.management.config.file=xxxx is set.  What is the current behavior?

I'm not sure whether you may run into some issue and hence setting 
system properties.

>> In addition, as specified in CSR, e.e.g
>>
>> --start-management-agent port=1234:configFile=management.properties:ssl=true:authenticate=false
>>
>> The value specified via the command line takes precedence over the value
>> specified in the config file.  port=1234 and ssl=true will take precedence
>> even if those properties are set in management.properties.
>>
>> It seems that this is not covered (or I missed it from the webrev).
> That is a default behavior for all command line flags and hence not 
> part of the webrev.

My comment is specific to 
"port=1234:configFile=management.properties:ssl=true:authenticate=false" 
where the value contains port set to 1234, ssl set to true, authenticate 
set to false explicitly.   The issue is about management.properties also 
specifies the properties for port, ssl, authenticate and the resulting 
value should be the explicit value specified in command line and then  
the properties listed in management.properties.

The argument parsing of --start-management-agent is done by Agent 
class.   So it should be covered by this change.  Please add a test case 
and you can confirm if it works.

>> ConnectorBootstrap.PropertyNames  defines the property names.  It may be
>> better to extend this class to take the short name and value validator
>> into account (replace the managementMap and validatorMap).  You may
>> want to refactor it out as an outer class if needed.
>
> ConnectorBootstrap.PropertyNames is heavily used in sources as well as 
> tests and hence it is not straight-forward to re factor it. I have 
> used the constants from the latter instead of raw strings for 
> managementMap.

I only found two tests referring to PropertyNames class:
    test/jdk/sun/management/jmxremote/bootstrap/RmiBootstrapTest.java 
and RmiSslNoKeyStoreTest.java

I will suggest to add a new class for your change and review.   Once we 
are happy with the new class, then separate the refactoring/renaming in 
a separate patch and JBS issue.  I think that should be straight 
forward.  JDK-8187498 will contain its own change.

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