samarthjain commented on code in PR #17847:
URL: https://github.com/apache/druid/pull/17847#discussion_r2082568948
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -40,9 +37,7 @@ public class SegmentGenerationMetrics
private final AtomicLong persistBackPressureMillis = new AtomicLong(0);
private final AtomicLong failedPersists = new AtomicLong(0);
private final AtomicLong failedHandoffs = new AtomicLong(0);
- // Measures the number of rows that have been merged. Segments are merged
into a single file before they are pushed to deep storage.
Review Comment:
nit: doesn't look related.
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -23,14 +23,11 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.ReentrantLock;
-/**
Review Comment:
nit: doesn't look related.
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -105,14 +140,32 @@ public void setSinkCount(long sinkCount)
this.sinkCount.set(sinkCount);
}
+ public void reportMessageGap(final long messageGap)
+ {
+ lock.lock();
+ try {
Review Comment:
This block of could be moved to inside MessageGapStats class.
And once you do that, I think a better approach would be to simply make
MessageGapStats class thread safe i.e. add synchronized on all the methods
inside the class. I would also then mark the class as @ThreadSafe.
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -40,9 +37,7 @@ public class SegmentGenerationMetrics
private final AtomicLong persistBackPressureMillis = new AtomicLong(0);
private final AtomicLong failedPersists = new AtomicLong(0);
private final AtomicLong failedHandoffs = new AtomicLong(0);
- // Measures the number of rows that have been merged. Segments are merged
into a single file before they are pushed to deep storage.
private final AtomicLong mergedRows = new AtomicLong(0);
- // Measures the number of rows that have been pushed to deep storage.
Review Comment:
nit: doesn't look related.
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -52,9 +47,49 @@ public class SegmentGenerationMetrics
private final AtomicLong messageMaxTimestamp = new AtomicLong(0);
private final AtomicLong messageGap = new AtomicLong(0);
private final AtomicBoolean processingDone = new AtomicBoolean(false);
-
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 double avgMessageGap()
+ {
+ return totalMessageGap / numMessageGap;
+ }
+
+ public long getMinMessageGap()
+ {
+ return minMessageGap;
+ }
+
+ public long getMaxMessageGap()
+ {
+ return maxMessageGap;
+ }
+
+ public long getNumMessageGap()
+ {
+ return numMessageGap;
+ }
+
+ public MessageGapStats copyOf()
+ {
+ final MessageGapStats copy = new MessageGapStats();
+ copy.minMessageGap = minMessageGap;
+ copy.maxMessageGap = maxMessageGap;
+ copy.numMessageGap = numMessageGap;
+ copy.totalMessageGap = totalMessageGap;
+ return copy;
+ }
+ }
+
+ private final MessageGapStats messageGapStats = new MessageGapStats();
+ private final ReentrantLock lock = new ReentrantLock();
Review Comment:
You don't need a separate lock for this.
Calls that access messageGapStats can be locked by using the internal object
lock itself.
`synchronized(messageGapStats) { .... }`
and then I would add a @GuardedBy("messageGapStats") annotation.
`@GuardedBy("messageGapStats")`
`private final MessageGapStats messageGapStats = new MessageGapStats();`
--
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]
