Re: PING: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"

[Chris Plummer]

Hi Yasumasa,

I took at look at it, as I did your initial weberv. Although I didn't 
see any issues, this is my first time looking at any of this code. It 
would be nice to have someone with more experience with this code than I 
have take a look at it, but I don't see that as a requirement as long as 
Stefan is confident in his review.


thanks,

Chris

On 4/25/18 5:48 AM, Yasumasa Suenaga wrote:

PING: Could you review this change?
I've sent review request about a month ago, but I do not yet get 
second reviewer.



    > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/



Yasumasa


On 2018/04/10 20:10, Yasumasa Suenaga wrote:

PING: Could you review it?
We need one more reviewer.


    > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/



Yasumasa


On 2018/04/03 21:37, Yasumasa Suenaga wrote:

PING: Could you review it?
This change has been passed Mach5 test.


    > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/



Thanks,

Yasumasa


On 2018/03/28 22:38, Stefan Johansson wrote:

Mach5 testing looks good.

Can someone in the serviceability team do the second review?

Cheers,
Stefan

On 2018-03-28 13:32, Yasumasa Suenaga wrote:

Thanks Stefan,
I'm waiting for second reviewer.


Yasumasa


2018年3月28日(水) 18:36 Stefan Johansson >:


    Hi Yasumasa,

    Local testing looks good and I've kicked of some additional Mach5
    testing that will include these tests on all platforms.

    Cheers,
    Stefan

    On 2018-03-28 06:04, Yasumasa Suenaga wrote:
    > Hi Stefan,
    >
    > Thank you for sharing your report!
    > I could reproduce them on my VM.
    >
    > I've fixed them in new webrev, and it works fine on my 
environment.

    > Could you check again?
    >
    > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/ 


    >
    >
    > Thanks,
    >
    > Yasumasa
    >
    >
    >
    > 2018-03-28 0:29 GMT+09:00 Stefan Johansson 
>:

    >>
    >> On 2018-03-27 16:44, Yasumasa Suenaga wrote:
    >>> Hi Stefan,
    >>>
    >>> On 2018/03/27 22:45, Stefan Johansson wrote:
     Hi Yasumasa,
    
     On 2018-03-27 10:56, Yasumasa Suenaga wrote:
    > Hi Stefan,
    >
    > Thank you for your comment.
    > I updated webrev:
    >
    >     webrev: 
http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.01/ 

     I think the usage of Optional in 
Expression.setRequired(bool) is a bit
     unnecessary. It will create temporary objects and there 
is no benefit from

     just doing two simple if-statements.
    >>>
    >>> I fixed it in new webrev:
    >>> 
http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.02/ 


    >>>
    >>>
     I also ran this patch (and the one using forcibly) on my 
single core VM
     and realized that this fix will have to include some 
awk-file updates to
     make the test in test/jdk/sun/tools/jstat pass when 
Serial in chosen as the
     default collector. The tests in 
test/jdk/sun/tools/jstatd/ are fine.

    >>>
    >>> Can you share the failure report?
    >> It relates to all tests that display the the CGC and the 
CGCT columns, for

    >> example in jstatGCOutput1.sh:
    >>   S0C    S1C    S0U    S1U      EC       EU OC  OU      
 MC     MU

    >> CCSC   CCSU   YGC     YGCT FGC    FGCT    CGC CGCT     GCT
    >> 256.0  256.0  254.0   0.0    2176.0   1025.0 5504.0 920.5  
  7168.0
    >> 6839.7 768.0  602.8       2    0.007   0 0.000   -       -  
  0.007

    >>
    >> The awk regex needs to be updated to handle '-' for these 
tests:

    >> test: sun/tools/jstat/jstatGcCapacityOutput1.sh
    >> Failed. Execution failed: exit code 1
    >>
    >> test: sun/tools/jstat/jstatGcMetaCapacityOutput1.sh
    >> Failed. Execution failed: exit code 1
    >>
    >> test: sun/tools/jstat/jstatGcNewCapacityOutput1.sh
    >> Failed. Execution failed: exit code 1
    >>
    >> test: sun/tools/jstat/jstatGcOldCapacityOutput1.sh
    >> Failed. Execution failed: exit code 1
    >>
    >> test: sun/tools/jstat/jstatGcOldOutput1.sh
    >> Failed. Execution failed: exit code 1
    >>
    >> test: sun/tools/jstat/jstatGcOutput1.sh
    >> Failed. Execution failed: exit code 1
    >>
    >>
    >>> If it occurs in jstatClassloadOutput1.sh, it relates to 
JDK-8173942.

    >>>
    >>>
    >>> Thanks,
    >>>
    >>> Yasumasa
    >>>
    >>>
     Thanks,
     Stefan
    >     submit-hs: 
mach5-one-ysuenaga-JDK-8199519-20180327-0652-16322

    >
    >
    > Thanks,
    >
    > Yasumasa
    >
    >
    >
    > 2018-03-27 0:03 GMT+09:00 Stefan Johansson
    > 

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

[mandy chung]

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  and  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= 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

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

[Harsha Wardhana B]

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  and  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 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