> On May 31, 2018, at 11:36 PM, mandy chung <mandy.ch...@oracle.com> wrote: > > Hi Bob, > > On 5/30/18 12:45 PM, Bob Vandette wrote:> >> RFE: Container Metrics >> https://bugs.openjdk.java.net/browse/JDK-8203357 >> WEBREV: >> http://cr.openjdk.java.net/~bobv/8203357/webrev.00
Thanks for the review, here an updated webrev: http://cr.openjdk.java.net/~bobv/8203357/webrev.01/ > > Looks fine in general. It's good to have this internal API ready > for JFR and other library code to use. > > I skimmed through the new tests. It'd be good to add some comments > to describe what it does (for example, set up a docker image etc). DockerTestUtils.java does contain some comments describing what the utility functions do. I’ll add a brief comment in TestDockerCpuMetrics.java and TestDockerMemoryMetrics.java explaining the test process. > > launcher.properties > 154 \ -XshowSettings:system (Linux Only) show host system or container\n\ > 155 \ configuration and continue\n\ > > A newline can be placed after -XshowSettings:system consistent with > other suboptions. Done. I also added a newline after the vm sub-option. > > test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java > > There are several long lines in the new test files such as: > MetricsCpuTester.java > MetricsMemoryTester.java > MetricsTester.java > > It'd help future side-by-side review if they are wrapped. I think > most of them are the construction of an exception. Fixed. > > I see a pattern of a name after @test and that is not strictly needed. > > TestCgroupMetrics.java > 25 * @test TestCgroupMetrics > > TestDockerCpuMetrics.java > 34 * @test TestSystemMetrics > > TestDockerMemoryMetrics.java > 30 * @test TestSystemMetrics > > TestSystemMetrics.java > 25 * @test TestSystemMetrics Remove the names after @test. > > This needs a CSR for the new -XshowSettings:system option. I filed a CSR for this a few days ago. https://bugs.openjdk.java.net/browse/JDK-8204107 Bob. > > Mandy