On Mon, 17 Feb 2025 11:28:38 GMT, Severin Gehwolf <[email protected]> wrote:
>> Sergey Chernyshev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> added details in the log message
>
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 60:
>
>> 58:
>> 59: Common.prepareWhiteBox();
>> 60: DockerTestUtils.buildJdkContainerImage(imageName);
>
> This can be moved out of the condition. It's there in both cases.
Done
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 85:
>
>> 83: } else {
>> 84: System.out.println("Metrics are from neither cgroup v1 nor
>> v2, skipped for now.");
>> 85: }
>
> Suggestion:
>
> throw new RuntimeException("cgroup version not known? was: " +
> metrics.getProvider());
I made it with `jtreg.SkippedException`
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 98:
>
>> 96: opts.addDockerOpts("--privileged")
>> 97: .addDockerOpts("--cgroupns=" + (privateNamespace ? "private"
>> : "host"))
>> 98: .addDockerOpts("--memory", "1g");
>
> Please extract this "larger" memory limit out of this function. The
> expectation is to have:
>
>
> Container memory limit => X
> Some lower limit in a moved path => Y
>
> Expect that the lower limit of Y is being detected.
>
>
> For example:
>
>
> testMemoryLimitSubgroupV1("1g", "100m", "104857600", false);
done.
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 118:
>
>> 116: opts.addDockerOpts("--privileged")
>> 117: .addDockerOpts("--cgroupns=" + (privateNamespace ?
>> "private" : "host"))
>> 118: .addDockerOpts("--memory", "1g");
>
> Same here with the `1g` container memory limit. Move it to a parameter.
done. extracted the parameter.
> test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetricsSubgroup.java
> line 72:
>
>> 70: } else {
>> 71: System.out.println("Metrics are from neither cgroup v1 nor
>> v2, skipped for now.");
>> 72: }
>
> Please `throw` here.
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966094564
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966094301
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966093295
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966092984
PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1966092288
