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
