Dennis-Mircea commented on code in PR #27899:
URL: https://github.com/apache/flink/pull/27899#discussion_r3129499245
##########
flink-runtime/src/main/java/org/apache/flink/runtime/instance/HardwareDescription.java:
##########
@@ -150,7 +150,7 @@ public String toString() {
//
--------------------------------------------------------------------------------------------
public static HardwareDescription extractFromSystem(long managedMemory) {
- final int numberOfCPUCores = Hardware.getNumberCPUCores();
+ final double numberOfCPUCores = Hardware.getNumberCPUCoresAsDouble();
Review Comment:
Good point. A truly irrational CPU share isn't actually expressible through
cgroups: the kernel stores `quota / period` in microseconds, so a user request
like "one-third of a CPU" ends up on disk as something like `33333 / 100000`.
The realistic equivalents are therefore truncated decimals such as `0.3` or
`0.33333`.
I added tests in HardwareTest covering both:
- `testCgroupV{1,2}SubCoreLimit` - typical sub-core input: `30000 / 100000`
-> returns `0.3` exactly.
- `testCgroupV{1,2}RepeatingFractionalLimit` /
`testCgroupV1OneThirdCpuLimit` - the "one-third" case: `33333 / 100000` ->
returns `0.33333`, also asserted to stay within `1e-4` of the mathematical `1.0
/ 3.0`
##########
flink-runtime/src/main/java/org/apache/flink/runtime/instance/HardwareDescription.java:
##########
@@ -141,7 +141,7 @@ public int hashCode() {
@Override
public String toString() {
return String.format(
- "cores=%d, physMem=%d, heap=%d, managed=%d",
+ "cores=%s, physMem=%d, heap=%d, managed=%d",
Review Comment:
Thanks for the thorough review! Going through each point:
### 🟠HIGH - Division by zero in cgroup v1
This is already guarded. In `Hardware#getCpuLimitFromCgroupV1`, both values
are validated before the division:
```java
if (quota <= 0) { ... return -1; } // v1 "-1" sentinel + negative/zero
guard
if (period <= 0) { ... return -1; } // explicit zero/negative period
guard
double cpuLimit = (double) quota / period;
```
The same is true for v2 (`if (quota > 0 && period > 0)`). No change needed.
### Missing unit tests / fractional test fixtures
Addressed. The two cgroup helpers are now `@VisibleForTesting` and take
injected `Path` parameters, which lets `HardwareTest` cover them with temp
files. Added 13 new tests covering: fractional limits (`0.5`, `1.5`), integer
limits (`2.0`), `"max"` unlimited (v2), `-1` unlimited (v1), zero period,
malformed content, non-numeric values, and missing files.
### cgroup v2 hybrid mode
The existing v2 -> v1 fallback already covers this. In hybrid mode the `cpu`
controller typically stays on the v1 hierarchy, so the v1 fallback picks it up
correctly. Added a note to the `getContainerCpuLimit` Javadoc to make this
explicit.
### Docs: `getNumberCPUCores()` vs `getNumberCPUCoresAsDouble()`
The guidance already existed on `getNumberCPUCoresAsDouble` ("Use this
method when the fractional value matters… For call sites that need an integer,
use `getNumberCPUCores()` instead"). Added the reverse cross-reference on
`getNumberCPUCores` as well.
### Thread pool 4× multiplier may over-provision for fractional CPUs
The only thread-pool site updated by this PR is
`ClusterEntrypointUtils#getPoolSize` (cluster IO executor), which now computes:
```java
(int) Math.ceil(4 * Hardware.getNumberCPUCoresAsDouble())
```
This actually reduces over-provisioning compared to the previous `4 *
getNumberCPUCores()`: on a container limited to 0.5 CPU, the old code produced
`4 * 1 = 4` threads, while the new code produces `ceil(4 * 0.5) = 2`. The
multiplication happens before the cast to `int`, so fractional limits are
honored rather than rounded up first. All other thread-pool sites still use the
unchanged `int` variant (`getNumberCPUCores`).
### Magic string `"max"`
Extracted to `Hardware.CGROUP_V2_UNLIMITED`.
### Redundant file existence checks
This is kept on purpose as they avoid throwing and logging
`NoSuchFileException` on the common non-Linux / non-containerized path, which
would be noisy in normal operation.
### Format specifier inconsistency in `toString()`
Fixed: `HardwareDescription#toString` now uses `%.2f` for `numberOfCPUCores`
instead of `%s`.
### Path traversal validation
N/A in this case. The cgroup paths are hardcoded `private static final
String` constants (`/sys/fs/cgroup/cpu.max`,
`/sys/fs/cgroup/cpu/cpu.cfs_quota_us`, `/sys/fs/cgroup/cpu/cpu.cfs_period_us`).
There is no user-supplied input in the path construction.
Let me know if anything's unclear or you'd like a different approach on any
of the above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
