jtuglu-netflix commented on code in PR #17847:
URL: https://github.com/apache/druid/pull/17847#discussion_r2094610674
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -105,14 +164,23 @@ public void setSinkCount(long sinkCount)
this.sinkCount.set(sinkCount);
}
+ public void reportMessageGap(final long messageGap)
+ {
+ messageGapStats.add(messageGap);
+ }
+
public void reportMessageMaxTimestamp(long messageMaxTimestamp)
{
- this.messageMaxTimestamp.set(Math.max(messageMaxTimestamp,
this.messageMaxTimestamp.get()));
+ if (this.messageMaxTimestamp.get() < messageMaxTimestamp) {
+ this.messageMaxTimestamp.getAndAccumulate(messageMaxTimestamp,
Math::max);
+ }
}
public void reportMaxSegmentHandoffTime(long maxSegmentHandoffTime)
{
- this.maxSegmentHandoffTime.set(Math.max(maxSegmentHandoffTime,
this.maxSegmentHandoffTime.get()));
+ if (this.maxSegmentHandoffTime.get() < maxSegmentHandoffTime) {
+ this.maxSegmentHandoffTime.getAndAccumulate(maxSegmentHandoffTime,
Math::max);
Review Comment:
Not sure I understand. This codepath does already have multi-threaded write
access (for `maxSegmentHandoffTime`). See `reset()`.
1. Not only is it making the code faster, it's also making removing an
incorrect, racy pattern.
2. See below benchmarks that it also provides a performance boost(3.5x+):
5 warmup/5 measurement on 10k calls
Old:
```
Benchmark
Mode Cnt Score Error Units
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMaxSegmentHandoffTime
avgt 5 22755.902 ± 80.865 ns/op
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMessageMaxTimestamp
avgt 5 22768.625 ± 63.275 ns/op
```
New:
```
Benchmark
Mode Cnt Score Error Units
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMaxSegmentHandoffTime
avgt 5 6254.388 ± 319.197 ns/op
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMessageGap
avgt 5 32911.301 ± 21.004 ns/op
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMessageMaxTimestamp
avgt 5 6252.912 ± 222.810 ns/op
```
##########
server/src/main/java/org/apache/druid/segment/realtime/SegmentGenerationMetrics.java:
##########
@@ -105,14 +164,23 @@ public void setSinkCount(long sinkCount)
this.sinkCount.set(sinkCount);
}
+ public void reportMessageGap(final long messageGap)
+ {
+ messageGapStats.add(messageGap);
+ }
+
public void reportMessageMaxTimestamp(long messageMaxTimestamp)
{
- this.messageMaxTimestamp.set(Math.max(messageMaxTimestamp,
this.messageMaxTimestamp.get()));
+ if (this.messageMaxTimestamp.get() < messageMaxTimestamp) {
+ this.messageMaxTimestamp.getAndAccumulate(messageMaxTimestamp,
Math::max);
+ }
}
public void reportMaxSegmentHandoffTime(long maxSegmentHandoffTime)
{
- this.maxSegmentHandoffTime.set(Math.max(maxSegmentHandoffTime,
this.maxSegmentHandoffTime.get()));
+ if (this.maxSegmentHandoffTime.get() < maxSegmentHandoffTime) {
+ this.maxSegmentHandoffTime.getAndAccumulate(maxSegmentHandoffTime,
Math::max);
Review Comment:
Not sure I understand. This codepath does already have multi-threaded write
access (for `maxSegmentHandoffTime`). See `reset()`.
1. Not only is it making the code faster, it's also making removing an
incorrect, racy pattern.
2. See below benchmarks that it also provides a performance boost(3.5x+
faster):
5 warmup/5 measurement on 10k calls
Old:
```
Benchmark
Mode Cnt Score Error Units
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMaxSegmentHandoffTime
avgt 5 22755.902 ± 80.865 ns/op
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMessageMaxTimestamp
avgt 5 22768.625 ± 63.275 ns/op
```
New:
```
Benchmark
Mode Cnt Score Error Units
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMaxSegmentHandoffTime
avgt 5 6254.388 ± 319.197 ns/op
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMessageGap
avgt 5 32911.301 ± 21.004 ns/op
SegmentGenerationMetricsBenchmark.benchmarkMultipleReportMessageMaxTimestamp
avgt 5 6252.912 ± 222.810 ns/op
```
--
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]
