Hi Severin,
On 2/11/20 10:04 AM, Severin Gehwolf wrote:
Updated webrev:
Full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/10/webrev/
incremental:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/10/incremental/webrev/
Thanks for updating this. This patch looks okay in general while I
still think this is quite hard to tell whether the implementation
matches the spec in the unavailable & unlimited cases. It'd be good if
this can be cleaned up in the future. Thanks for the new test.
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
92 logger.log(Level.WARNING, "Mixed cgroupv1 and cgroupv2 not supported.
Metrics disabled.");
There isn't a clear guideline on what logging level to use. I will opt
for API not emit any message as this method returns null. or this
should throw an exception if this should be reported instead. So debug
or trace level seems to be suitable for this message.
130 return new CgroupMetrics(new CgroupV2Subsystem(unified));
131 } else {
132 return new CgroupV1Metrics(CgroupV1Subsystem.getInstance());
CgroupV2Subsystem is instantiated with a constructor. Do you expect
multiple instances of CgroupV2Subsystem (as opposed to CgroupV1Subsystem
is a singleton)?
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java
391 String match = "hierarchical_memory_limit";
392 retval = CgroupV1SubsystemController.getLongValueMatchingLine(memory,
393 "memory.stat",
394 match); Nit: the match variable is not needed. Similar pattern
occurs in line 451-453.
src/java.base/share/classes/jdk/internal/platform/MetricsCgroupV1.java
This is cgroup v1 specific. I still think it should be moved to
linux/classes rather than share/classes. The tests should only run on
linux.
src/java.base/linux/classes/jdk/internal/platform/CgroupV1Metrics.java
copyright header is missing
You can define a private MetricsCgroupV1 subsystem field in this
class to avoid the casting:
15 return ((MetricsCgroupV1)subsystem).getMemoryMaxUsage();
So CgroupMetrics::subsystem field can stay as private if desire.
Side note: CgroupV1Metrics is the implementation of MetricsCgroupV1.
The naming is a bit strange. CgroupV1Metrics sounds better for the
interface and the implementation class could be CgroupV1MetricsImpl and
it'd help a reader to understand their relationship. I leave it for you
to decide.
src/java.base/share/classes/sun/launcher/LauncherHelper.java
+ private static final long LONG_RETVAL_NOT_SUPPORTED = -2;
This is specific to printSystemMetrics. I suggest to move this to
printSystemMetrics as a local variable. Then formatCpuVal and
formatLimitString to take an additional "unavailable" parameter so that
these methods will print "N/A" if limit == unavailable.
+ private static String formatBoolean(Boolean value, String prefix) {
This is no longer needed. Please remove it.
test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java
The Oracle copyright is taken out and the copyright is also changed
from GPL to GPL+CP.
The Red Hat copyright can be added to the top of the file
immediately before "DO NOT ALTER or REMOVE" line like [1].
[1]
http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitEnableDisable.java
test/jdk/jdk/internal/platform/docker/MetricsMemoryTester.java
TestDockerMemoryMetrics requires docker.support. I think any test
depending on MetricsMemoryTester.java are filtered when running on
platform other than linux. Please verify that.
test/lib/jdk/test/lib/containers/cgroup/CPUSetsReader.java
58 try(Stream<String> stream = Files.lines(Paths.get(path))) {
Nit: space between "try" and "(" is missing.
test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemController.java
I expect the copyright header be placed at the top of the file
before all imports.
thanks
Mandy
