gemini-code-assist[bot] commented on code in PR #38816: URL: https://github.com/apache/beam/pull/38816#discussion_r3358246993
########## runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/HotKeyLogger.java: ########## @@ -18,6 +18,7 @@ package org.apache.beam.runners.dataflow.worker; import com.google.api.client.util.Clock; +import javax.annotation.concurrent.GuardedBy; Review Comment:  Since we can use `AtomicLong` to achieve lock-free thread safety, the `GuardedBy` annotation is no longer needed. We can replace it with an import for `AtomicLong`. ```suggestion import java.util.concurrent.atomic.AtomicLong; ``` ########## runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/HotKeyLogger.java: ########## @@ -33,6 +34,7 @@ public class HotKeyLogger { * The previous time the HotKeyDetection was logged. This is used to throttle logging to every 5 * minutes. */ + @GuardedBy("this") private long prevHotKeyDetectionLogMs = 0; Review Comment:  Using `synchronized(this)` on a public/protected class is generally discouraged as it exposes the lock to external callers, which can lead to deadlocks or unexpected contention. Using `AtomicLong` provides a lock-free, thread-safe alternative. ```suggestion private final AtomicLong prevHotKeyDetectionLogMs = new AtomicLong(0); ``` ########## runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/HotKeyLogger.java: ########## @@ -83,10 +85,12 @@ public void logHotKeyDetection(String userStepName, Duration hotKeyAge, Object h protected boolean isThrottled() { // Throttle logging the HotKeyDetection to every 5 minutes. long nowMs = clock.currentTimeMillis(); - if (nowMs - prevHotKeyDetectionLogMs < loggingPeriod.getMillis()) { - return true; + synchronized(this) { + if (nowMs - prevHotKeyDetectionLogMs < loggingPeriod.getMillis()) { + return true; + } + prevHotKeyDetectionLogMs = nowMs; } Review Comment:  With `prevHotKeyDetectionLogMs` refactored to `AtomicLong`, we can implement lock-free throttling using `compareAndSet`. This avoids the need for a `synchronized` block entirely. ```suggestion long prevMs = prevHotKeyDetectionLogMs.get(); if (nowMs - prevMs < loggingPeriod.getMillis()) { return true; } if (!prevHotKeyDetectionLogMs.compareAndSet(prevMs, nowMs)) { return true; } ``` -- 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]
