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

[Harsha Wardhana B]

On Thursday 26 April 2018 09:09 PM, mandy chung wrote:
>
>
> On 4/23/18 1:20 PM, Harsha Wardhana B wrote:
>> Hi All,
>>
>> After internal discussions, many of the concerns below were addressed 
>> and final spec is published at,
>>
>> https://bugs.openjdk.java.net/browse/JDK-8199584
>>
>> Below is the implementation of the above spec.
>>
>> http://cr.openjdk.java.net/~hb/8187498/webrev.05/
>>
> 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

It is also part of java -help output which has been already approved in 
the CSR.
>   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.
> src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java I 
> think the change from single class import to import java.util.* and 
> java.io.* may not be done intentionally (I guess it might be done by 
> IDE). I prefer to keep the orginal single class import.
ok.
>
> 665 if (args.equals("--start-management-agent") || 
> args.equals("--start-management-agent=")) { 666 // Options are 
> mandatory 667 error(INVALID_OPTION, args); 668 }
> 709 Optional<String> argStr = vmArguments.stream() 710 .filter(a -> 
> a.startsWith("--start-management-agent")) 711 .reduce((a, b) -> b); 
> The only form passed to the VM is --start-management-agent=.... since 
> VM option does not take white-space separated argument. I think line 
> 666 and 710 should check for "--start-management-agent=" only.
ok
> 676 String minusDflag = managementFlags.get(name); minusDflag can be 
> renamed to "propertyName" which is clearer.
ok
> 714 props.forEach((a, b) -> { 715 String oldVaue = 
> System.getProperty((String) a); 716 if (oldVaue != null && 
> !oldVaue.isEmpty()) { 717 error(INVALID_OPTION, "management options 
> can be set via -D flags or via --start-management-agent but not 
> both"); 718 } 719 System.setProperty((String) a, (String) b); 720 }); 
> 721 }  714 props.forEach((a, b) -> {
> 715 String oldVaue = System.getProperty((String) a);
> 716 if (oldVaue != null && !oldVaue.isEmpty()) {
> 717 error(INVALID_OPTION, "management options can be set via -D flags 
> or via --start-management-agent but not both");
> 718 }
> I think checking if -D is set should be done for each constant defined 
> in ConnectorBootstrap.PropertyNames class instead of each key in props 
> since it could set -Dcom.sun.management.jmxremote.port it didn't set 
> port in --start-management-agent. Do you have such test case covered?
Agreed. I will modify the code and add a test case to validate the changes.
>   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.
> 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.
> 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 will review the test when the webrev is updated (in case the test will
> need update).
>
> Mandy
>
Thanks
Harsha

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