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
