scwhittle commented on code in PR #34214:
URL: https://github.com/apache/beam/pull/34214#discussion_r1986897263
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ExecutionStateTracker.java:
##########
@@ -150,7 +150,7 @@ public String getDescription() {
private long transitionsAtLastSample = 0;
private long nextLullReportMs = LULL_REPORT_MS;
- private long nextBundleLullDurationReportMs = BUNDLE_LULL_REPORT_MS;
+ private volatile long nextBundleLullDurationReportMs = BUNDLE_LULL_REPORT_MS;
Review Comment:
instead what about moving nextBundleLullDurationReportMs to be reset in
reset(), next to nextLullReportMs (which seems like it should have similar
synchronization).
The race is because deactivate.removeTracker() doesn't actually mean that
the tracker won't be sampled again as it internally uses a concurrent map.
However nextLullReportMs would have the same raciness with sampling, so I think
moving the nextBundleLullDurationReportMs to the same location would avoid the
race as well without the additional overhead of another volatile on the
sampling path.
--
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: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
