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

[mandy chung]

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.

  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" 
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.
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. 676 String 
minusDflag = managementFlags.get(name); minusDflag can be renamed to 
"propertyName" which is clearer. 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?   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?

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

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.

I will review the test when the webrev is updated (in case the test will
need update).

Mandy

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