[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.
From: mandy chung <mandy.chung () oracle ! com>
Date: 2018-04-27 8:38:17
Message-ID: b1f0074e-0833-f68f-e183-f278f640b42d () oracle ! com
[Download RAW message or body]
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
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 4/27/18 3:43 PM, Harsha Wardhana B
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:9a473153-439b-dbda-dddb-084abe7ef61c@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<br>
<br>
<div class="moz-cite-prefix">On Thursday 26 April 2018 09:09 PM,
mandy chung wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e403893e-9543-0df3-ccf0-1b695350d529@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
src/java.base/share/classes/sun/launcher/resources/launcher.properties<br>
<pre><span class="new"> 112 \ --start-management-agent \
option=value[:option=value:....]\n\</span>
option and value should be <option> and <value> to represent \
user-supplied name/value.</pre> </blockquote>
The syntax used is borrowed from Java tool guide, <br>
<br>
<a class="moz-txt-link-freetext"
href="https://docs.oracle.com/javase/10/tools/java.htm#JSWOR624"
moz-do-not-send="true">https://docs.oracle.com/javase/10/tools/java.htm#JSWOR624</a><br>
</blockquote>
<br>
I checked java -h output. <...> is the format for variables.<br>
<blockquote type="cite"
cite="mid:9a473153-439b-dbda-dddb-084abe7ef61c@oracle.com"> <br>
It is also part of java -help output which has been already
approved in the CSR.<br>
</blockquote>
<br>
This is minor editing. Most importantly the output of java -help
should be consistent.<br>
<br>
<blockquote type="cite"
cite="mid:9a473153-439b-dbda-dddb-084abe7ef61c@oracle.com">
<blockquote type="cite"
cite="mid:e403893e-9543-0df3-ccf0-1b695350d529@oracle.com">
<pre> <span class="new">113 \ start the default management \
agent with semi-colon separated\n\</span> <span class="new">114 \ \
list of options. Multiple values for an option should be separated\n\</span> <span \
class="new"> 115 \ by comma. See the java tool guide for a list of \
options for\n\</span> <span class="new"> 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"</span></pre>
</blockquote>
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. <br>
</blockquote>
<br>
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.<br>
<blockquote type="cite"
cite="mid:9a473153-439b-dbda-dddb-084abe7ef61c@oracle.com">
<blockquote type="cite"
cite="mid:e403893e-9543-0df3-ccf0-1b695350d529@oracle.com">
<pre><span class="new"></span><span class="new"></span> </pre>
</blockquote>
<blockquote type="cite"
cite="mid:e403893e-9543-0df3-ccf0-1b695350d529@oracle.com">
<pre><span class="new">
</span> <span class="new">719 System.setProperty((String) a, \
(String) b);</span><span class="new">
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.
</span>I see the existing code calling System.getProperty is not modified. I think
that may need to be updated too?</pre>
</blockquote>
I did not understand. Can you please elaborate.<br>
</blockquote>
<br>
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?<br>
<br>
I'm not sure whether you may run into some issue and hence setting
system properties. <br>
<br>
<blockquote type="cite"
cite="mid:9a473153-439b-dbda-dddb-084abe7ef61c@oracle.com">
<blockquote type="cite"
cite="mid:e403893e-9543-0df3-ccf0-1b695350d529@oracle.com">
<pre>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).</pre>
</blockquote>
That is a default behavior for all command line flags and hence
not part of the webrev.<br>
</blockquote>
<br>
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.<br>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:9a473153-439b-dbda-dddb-084abe7ef61c@oracle.com">
<blockquote type="cite"
cite="mid:e403893e-9543-0df3-ccf0-1b695350d529@oracle.com">
<pre><span class="new"><span class="new"><span \
class="new">ConnectorBootstrap.PropertyNames</span></span></span> 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.</pre>
</blockquote>
<br>
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.<br>
</blockquote>
<br>
I only found two tests referring to PropertyNames class:<br>
test/jdk/sun/management/jmxremote/bootstrap/RmiBootstrapTest.java
and RmiSslNoKeyStoreTest.java<br>
<br>
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.<br>
<br>
Mandy<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic