[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 81820709 - Container Awareness JEP
From:       mandy chung <mandy.chung () oracle ! com>
Date:       2018-04-18 2:25:01
Message-ID: 4e73dfdf-54f5-eb43-5e7d-0cfb316fb9e7 () oracle ! com
[Download RAW message or body]

On 4/3/18 10:09 PM, Bob Vandette wrote:
> WEBREV:
>
> http://cr.openjdk.java.net/~bobv/8182070/v01/webrev

I reviewed the webrev and look okay in general. I will look through the 
javadoc next.

Metrics.java

   37  *<li> 1. All processes, including the current process within a container.

   <ol> includes the numbering. You can drop "1." and other numbers.
  
   42  *<li> or

This adds a bullet.  Maybe dropping this line.

   81      * @return The name of the provider or null if Metrics are
   82      *         not enabled.
   85     public String getProvider();

Should this method always return non-null name?

For optional metric (when it's not available), the method returns 0.  For example:
  533      * @return The number of bytes transferred or 0 if this metric is not available.

How does the client know if the metrics is not available or zero?  Or the client does not care?

jdk/internal/platform/cgroupv1/Metrics.java

  274         return SubSystem.getLongValue(cpuacct, "cpuacct.usage");

Should this be an instance method?  like cpuacct.getLongValue("cpuacct.usage");

final field name can be made all caps.

I know you are going to include regression tests.

>
> WEBREV including a Prototype MBEAN for exposing these Metrics:
>
> This prototype will not be integrated as part of this JEP.  It's for information only.
>
> http://cr.openjdk.java.net/~bobv/8182070/v01/mbean-proto/
>
>
> This feature adds a new -XshowSetting option "system" which displays the
> available system Metrics.

What does java --help-extra show?   The help message should include 
-XshowSettings:system only on Linux.

>
> % java -XshowSettings:system

I expect this option shows static/configuration information rather than 
timing statistics e.g. CPU time and usage.   It may be a smaller set but 
it may be good information though.

It's more appropriate for monitoring tools to show the timing statistics 
and resource consumption rather than the launcher.

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/3/18 10:09 PM, Bob Vandette wrote:
    </div>
    <blockquote type="cite"
      cite="mid:A88F8550-5DBA-4960-9772-85626101A3A7@oracle.com">
      <pre wrap="">
WEBREV:

<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~bobv/8182070/v01/webrev">http://cr.openjdk.java.net/~bobv/8182070/v01/webrev</a>
 </pre>
    </blockquote>
    <br>
    I reviewed the webrev and look okay in general. I will look through
    the javadoc next.<br>
    <pre>Metrics.java 

  37  *&lt;li&gt; 1. All processes, including the current process within a container.

  &lt;ol&gt; includes the numbering. You can drop "1." and other numbers.
 
  42  *&lt;li&gt; or

This adds a bullet.  Maybe dropping this line.

  81      * @return The name of the provider or null if Metrics are 
  82      *         not enabled.
  85     public String getProvider();

Should this method always return non-null name?

For optional metric (when it's not available), the method returns 0.  For example:
 533      * @return The number of bytes transferred or 0 if this metric is not \
available.

How does the client know if the metrics is not available or zero?  Or the client does \
not care?

jdk/internal/platform/cgroupv1/Metrics.java

 274         return SubSystem.getLongValue(cpuacct, "cpuacct.usage");

Should this be an instance method?  like cpuacct.getLongValue("cpuacct.usage");

final field name can be made all caps.

I know you are going to include regression tests.

</pre>
    <blockquote type="cite"
      cite="mid:A88F8550-5DBA-4960-9772-85626101A3A7@oracle.com">
      <pre wrap="">

WEBREV including a Prototype MBEAN for exposing these Metrics:

This prototype will not be integrated as part of this JEP.  It's for information \
only.

<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~bobv/8182070/v01/mbean-proto/">http://cr.openjdk.java.net/~bobv/8182070/v01/mbean-proto/</a>



This feature adds a new -XshowSetting option "system" which displays the
available system Metrics.   </pre>
    </blockquote>
    <br>
    What does java --help-extra show?   The help message should include
    -XshowSettings:system only on Linux.   <br>
    <br>
    <blockquote type="cite"
      cite="mid:A88F8550-5DBA-4960-9772-85626101A3A7@oracle.com">
      <pre wrap="">

% java -XshowSettings:system
</pre>
    </blockquote>
    <br>
    I expect this option shows static/configuration information rather
    than timing statistics e.g. CPU time and usage.   It may be a smaller
    set but it may be good information though.<br>
    <br>
    It's more appropriate for monitoring tools to show the timing
    statistics and resource consumption rather than the launcher.<br>
    <br>
    Mandy<br>
    <br>
  </body>
</html>



[prev in list] [next in list] [prev in thread] [next in thread]

Configure | About | News | Add a list | Sponsored by KoreLogic