morningman commented on PR #60777:
URL: https://github.com/apache/doris/pull/60777#issuecomment-4017317548
## 1. [Critical] Split Unrelated Changes into Separate PRs
This PR bundles two completely unrelated changes:
- **CPU metrics enhancement** in `SystemMetrics` / `MetricRepo`
- **`ConcurrentModificationException` bug fix** in `PartitionCompensator`
Mixing unrelated changes in a single PR makes review harder, complicates git
blame/bisect, and increases the risk of unintended regressions. These should be
split into two independent PRs:
- PR A: `[Enhancement](metrics) Added CPU usage metrics`
- PR B: `[Fix](mv) Fix ConcurrentModificationException in
PartitionCompensator`
---
## 2. [Medium] Improve Unit Test Coverage for CPU Percentage Calculation
The current `testCpuMetrics()` test cannot verify that the percentage
calculation logic is correct. Since `MetricRepo.init()` calls `updateMetrics()`
only once, it is always the "first call" — `prevTotal` is 0, so all percentage
values default to 0.0. The assertion `>= 0.0` passes trivially but does not
validate functional correctness.
**Suggestion**: call `update()` twice to establish a delta, then verify the
percentage values against expected results computed from the test data.
```java
@Test
public void testCpuPercentageCalculation() {
SystemMetrics metrics = new SystemMetrics();
// First update establishes the baseline
metrics.update();
// Second update computes percentages based on delta
metrics.update();
// With static test data (stat_normal), both snapshots are identical,
// so delta = 0 and percentages remain 0.
// Ideally, use two different test data files to simulate a real delta
scenario.
}
```
Also, `process_forks_rate` is asserted as `>= 0.0`, which always passes even
though the value is always exactly 0 (due to the missing `processes` parsing).
The test should validate the actual expected value after the parsing bug is
fixed.
---
## 3. [Medium] Reduce Code Duplication in Line Parsing
The parsing logic for `ctxt`, `procs_running`, `procs_blocked` (and the
missing `processes`) follows an identical pattern: split the line by
whitespace, check length, and parse the second token. This can be extracted
into a helper method:
```java
private long parseSingleLongFromLine(String line) {
String[] parts = line.split("\\s+");
return parts.length >= 2 ? Long.parseLong(parts[1]) : 0;
}
```
Usage:
```java
} else if (line.startsWith("ctxt ")) {
ctxt = parseSingleLongFromLine(line);
} else if (line.startsWith("processes ")) {
processes = parseSingleLongFromLine(line);
} else if (line.startsWith("procs_running ")) {
procsRunning = parseSingleLongFromLine(line);
} else if (line.startsWith("procs_blocked ")) {
procsBlocked = parseSingleLongFromLine(line);
}
```
---
## 4. [Low] Fix Rate Calculation Guard Condition
The current condition for calculating context switch and process fork rates
is:
```java
if (timeDelta > 0 && prevCtxt > 0 && prevProcesses > 0) {
```
The `&&` relationship means that if **either** `prevCtxt` or `prevProcesses`
is 0, **both** rate calculations are skipped. This is problematic because:
1. Even after fixing the `processes` parsing bug, `prevProcesses` will be 0
on the first call, which also blocks `ctxtRate` calculation.
2. `ctxtRate` and `processesRate` are independent — one should not prevent
the other.
**Suggested fix** — either use separate guards or use a general "is first
call" flag:
```diff
-if (timeDelta > 0 && prevCtxt > 0 && prevProcesses > 0) {
- double timeInSeconds = timeDelta / 1000.0;
- ctxtRate = (ctxt - prevCtxt) / timeInSeconds;
- processesRate = (processes - prevProcesses) / timeInSeconds;
-}
+if (timeDelta > 0) {
+ double timeInSeconds = timeDelta / 1000.0;
+ if (prevCtxt > 0) {
+ ctxtRate = (ctxt - prevCtxt) / timeInSeconds;
+ }
+ if (prevProcesses > 0) {
+ processesRate = (processes - prevProcesses) / timeInSeconds;
+ }
+}
```
---
## 5. [Low] Consider Using Getter Methods for Field Access
All new fields in `SystemMetrics` are `protected` and accessed directly from
anonymous `GaugeMetric` classes in `MetricRepo` (e.g.,
`SYSTEM_METRICS.cpuUsagePercent`). Using getter methods would improve
encapsulation and maintainability:
```diff
-return SYSTEM_METRICS.cpuUsagePercent;
+return SYSTEM_METRICS.getCpuUsagePercent();
```
> **Note**: The existing TCP and memory metrics also use direct `protected`
field access, so maintaining consistency with the current codebase style is
also a valid choice.
---
## 6. [Low] Pre-compile Regex Pattern
`line.split("\\s+")` is called multiple times inside a loop. Each call
internally compiles the regex pattern. While the performance impact is
negligible for this use case, pre-compiling the pattern is a Java best practice:
```java
private static final Pattern WHITESPACE = Pattern.compile("\\s+");
// Usage:
String[] parts = WHITESPACE.split(line);
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
