Hi Mandy, I’m finally getting back to your review of this change now that we’ve made some progress on creating tests.
BTW: This Jira issue is now an RFE rather than a JEP (https://bugs.openjdk.java.net/browse/JDK-8203357 <https://bugs.openjdk.java.net/browse/JDK-8203357>) See comments below ... > On Apr 17, 2018, at 10:25 PM, mandy chung <mandy.ch...@oracle.com> wrote: > > > > On 4/3/18 10:09 PM, Bob Vandette wrote: >> WEBREV: >> >> http://cr.openjdk.java.net/~bobv/8182070/v01/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. Done > > 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? Done > > 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? Unfortunately this is the way that cgroups works. If the kernel has not been configured to provide the metrics, the values are all 0’s. I’m not sure what I can do about this behavior except document it as I have done in the JavaDocs. > > jdk/internal/platform/cgroupv1/Metrics.java > > 274 return SubSystem.getLongValue(cpuacct, "cpuacct.usage"); > > Should this be an instance method? like > cpuacct.getLongValue("cpuacct.usage”); I did it this way in order to provide a centralized place to check for missing subsystems. The getLongValue method does the checking for all subsystems. > > final field name can be made all caps. Not sure what this is in reference to, please advise? > > 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/ >> <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. The message looks like it comes out of a resource file will need to be localized. How do we make the message conditional on operating system in that case? Can I just put (Linux Only) in the english version and then get it localized? > >> >> % 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. I’ve removed any reporting of resource consumption APIs. Here’s the new output: ./java -XshowSettings:system Operating System Metrics: Provider: cgroupv1 Effective CPU Count: 24 CPU Period: 100000 CPU Quota: -1 CPU Shares: -1 List of Processors, 24 total: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 List of Effective Processors, 24 total: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 List of Memory Nodes, 2 total: 0 1 List of Available Memory Nodes, 2 total: 0 1 CPUSet Memory Pressure Enabled: false Memory Limit: Unlimited Memory Soft Limit: Unlimited Memory & Swap Limit: Unlimited Kernel Memory Limit: Unlimited TCP Memory Limit: 2048.00T Out Of Memory Killer Enabled: true I’ll be sending out a webrev that includes the tests next week once I’ve integrated them with my change and perform some testing on different Linux systems and docker containers. Thanks, Bob. > > Mandy >
