davidradl commented on code in PR #26257:
URL: https://github.com/apache/flink/pull/26257#discussion_r1983443544
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/SystemResourcesCounter.java:
##########
@@ -53,8 +53,8 @@ public class SystemResourcesCounter extends Thread {
private volatile boolean running = true;
- private long[] previousCpuTicks;
- private long[][] previousProcCpuTicks;
+ private volatile long[] previousCpuTicks;
Review Comment:
I had a look at the usages of these 2 fields
it seems `previousCpuTicks` is only [updated if it is
null](https://github.com/apache/flink/blob/45fcd56bbe4a8f7d4fc10985d5c148f8e234d9b2/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/SystemResourcesCounter.java#L195).
Which looks very strange - is this right? I can see that volatile could be
useful here if this is correct processing.
previousProcCpuTicks is updated when null then after processing in the
method see code
[here](https://github.com/apache/flink/blob/45fcd56bbe4a8f7d4fc10985d5c148f8e234d9b2/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/SystemResourcesCounter.java#L210).
I wonder if the
` previousCpuTicks` logic should update it, in the same way as
`previousProcCpuTicks `.
I was looking at these usages to see if AtomicReference should be used
rather than volatile. This would be more thread safe, and to use
compareAndSet() to update it.
--
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]
