jtuglu-netflix commented on code in PR #17847:
URL: https://github.com/apache/druid/pull/17847#discussion_r2094612845
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -55,6 +55,65 @@ public class SegmentGenerationMetrics
private final AtomicLong maxSegmentHandoffTime = new
AtomicLong(NO_EMIT_SEGMENT_HANDOFF_TIME);
+ public static class MessageGapStats
+ {
+ long minMessageGap = Long.MAX_VALUE;
+ long maxMessageGap = Long.MIN_VALUE;
+ long numMessageGap = 0;
+ double totalMessageGap = 0;
+
+ public synchronized double avgMessageGap()
Review Comment:
> Agreed. I don't see why the add needs to be synchronized either. Can it be
accessed by multiple threads?
Your atomic reference suggestions can allow a dangling reference to the
"old" gap statistics value that's being modified by the add() to leak into the
reporting loop's snapshot (in TaskRealtimeMetricsMonitor). This means unsafe
reads can happen in the cold reporting thread while the add() is using
protected writes on the same object in the hot Appenderator thread. This is why
I originally didn't want to use a reference, and added a thread-safe
`copyAndReset` to allow clean copy of the original object to be made during
snapshot, instead of passing a reference along. Atomic reference isn't
thread-safety if you're not using new values every time the atomic pointer is
updated.
--
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]
