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 while the add() is using protected writes on the same object.
This is why I originally didn't want to use a reference, and added a
thread-safe `copyAndReset` to allow clean copy to be made during snapshot.
Atomic reference isn't thread-safe if you're not using new values every time
the atomic pointer is updated.
##########
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 while the add() is using protected writes on the same object.
This is why I originally didn't want to use a reference, and added a
thread-safe `copyAndReset` to allow clean copy to be made during snapshot.
Atomic reference isn't thread-safe if you're not using new object 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]
