On Thu, 15 May 2025 16:21:56 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
>> PAWAN CHAWDHARY has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove unused import
>
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 73:
>
>> 71: return;
>> 72: }
>> 73: if (IS_DOCKER &&
>> ContainerRuntimeVersionTestUtils.DOCKER_VERSION_20_10_0.compareTo(ContainerRuntimeVersionTestUtils.getContainerRuntimeVersion())
>> > 0) {
>
> Better to replace this with
> `isContainerVersionSupported()`
> ...
> and implement all logic in the the ContainerRuntimeVersionTestUtils
created checkContainerVersionSupported()
> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java
> line 38:
>
>> 36: private final int micro;
>> 37: private static final ContainerRuntimeVersionTestUtils DEFAULT = new
>> ContainerRuntimeVersionTestUtils(0, 0, 0);
>> 38: public static final ContainerRuntimeVersionTestUtils
>> DOCKER_VERSION_20_10_0 = new ContainerRuntimeVersionTestUtils(20, 10, 0);
>
> Please add comment about meaning of the version or even better rename
> DOCKER_MINIMAL_SUPPORTED_VERSION = ....
updated name to DOCKER_MINIMAL_SUPPORTED_VERSION_CGROUPNS and
PODMAN_MINIMAL_SUPPORTED_VERSION_CGROUPNS
> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java
> line 53:
>
>> 51: } else if (this.major < other.major) {
>> 52: return -1;
>> 53: } else { // equal major
>
> no need to add `else {` here
updated
> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java
> line 58:
>
>> 56: } else if (this.minor < other.minor) {
>> 57: return -1;
>> 58: } else { // equal majors and minors
>
> no need to add `else {` here
updated
> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java
> line 81:
>
>> 79: } catch (Exception e) {
>> 80: System.out.println("Failed to parse container runtime
>> version: " + version);
>> 81: return DEFAULT;
>
> Any reason to don't fail here?
updated
> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java
> line 94:
>
>> 92: } catch (Exception e) {
>> 93: System.out.println(Container.ENGINE_COMMAND + " --version
>> command failed. Returning null");
>> 94: return null;
>
> Any reason to don't fail here?
updated
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099494838
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099496846
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099494004
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099494142
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099497419
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099497254
