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
