kfaraz commented on code in PR #20027:
URL: https://github.com/apache/druid/pull/20027#discussion_r3801069438
##########
server/src/main/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIterator.java:
##########
@@ -437,25 +422,56 @@ List<Interval> findInitialSearchInterval(SegmentTimeline
timeline, List<Interval
.map(segment -> segment.getId().getIntervalEnd())
.max(Comparator.naturalOrder())
.orElseThrow(AssertionError::new);
- searchIntervals.add(new Interval(searchStart, searchEnd));
+ final Interval searchInterval = new Interval(searchStart, searchEnd);
+ final Interval overlappingSkipInterval = allSkipIntervals.stream()
+
.filter(searchInterval::overlaps)
+ .findFirst()
+ .orElse(null);
+ // Guardrail check, this should never happen
+ if (overlappingSkipInterval != null) {
+ log.warn(
+ "searchInterval[%s] for datasource[%s] unexpectedly overlaps
skipInterval[%s]: %s, skipping it",
+ searchInterval, dataSource, overlappingSkipInterval,
+ describeSkipReason(overlappingSkipInterval, skipOffset,
config.getSkipIntervals(), skipIntervals)
+ );
+ continue;
+ }
+ searchIntervals.add(searchInterval);
}
return searchIntervals;
}
- static Interval computeLatestSkipInterval(
- @Nullable Granularity configuredSegmentGranularity,
- DateTime latestDataTimestamp,
- Period skipOffsetFromLatest
+ private static String describeSkipReason(
+ Interval skipInterval,
+ Period skipOffset,
+ List<Interval> configuredSkipIntervals,
+ List<Interval> lockedIntervals
)
{
- if (configuredSegmentGranularity == null) {
- return new Interval(skipOffsetFromLatest, latestDataTimestamp);
- } else {
- DateTime skipFromLastest = new DateTime(latestDataTimestamp,
latestDataTimestamp.getZone()).minus(skipOffsetFromLatest);
- DateTime skipOffsetBucketToSegmentGranularity =
configuredSegmentGranularity.bucketStart(skipFromLastest);
- return new Interval(skipOffsetBucketToSegmentGranularity,
latestDataTimestamp);
+ final StringBuilder reason = new StringBuilder(
+ StringUtils.format("interval[%s] overlaps one of the skip sources:
skipOffsetFromLatest[%s]", skipInterval, skipOffset)
+ );
+ if (!configuredSkipIntervals.isEmpty()) {
+ reason.append(", configured
skipIntervals").append(configuredSkipIntervals);
+ }
+ if (!lockedIntervals.isEmpty()) {
+ reason.append(", locked intervals").append(lockedIntervals);
+ }
+ return reason.toString();
+ }
+
+ private static Interval alignToSegmentGranularity(@Nullable Granularity
segmentGranularity, Interval interval)
Review Comment:
Super nit: This method need not be static.
Then you can avoid passing the target segment granularity repeatedly, and
simplify the lambda to `this::alignToSegmentGranularity`.
##########
server/src/main/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIterator.java:
##########
@@ -437,25 +422,56 @@ List<Interval> findInitialSearchInterval(SegmentTimeline
timeline, List<Interval
.map(segment -> segment.getId().getIntervalEnd())
.max(Comparator.naturalOrder())
.orElseThrow(AssertionError::new);
- searchIntervals.add(new Interval(searchStart, searchEnd));
+ final Interval searchInterval = new Interval(searchStart, searchEnd);
+ final Interval overlappingSkipInterval = allSkipIntervals.stream()
+
.filter(searchInterval::overlaps)
+ .findFirst()
+ .orElse(null);
+ // Guardrail check, this should never happen
+ if (overlappingSkipInterval != null) {
+ log.warn(
+ "searchInterval[%s] for datasource[%s] unexpectedly overlaps
skipInterval[%s]: %s, skipping it",
+ searchInterval, dataSource, overlappingSkipInterval,
+ describeSkipReason(overlappingSkipInterval, skipOffset,
config.getSkipIntervals(), skipIntervals)
+ );
+ continue;
+ }
+ searchIntervals.add(searchInterval);
}
return searchIntervals;
}
- static Interval computeLatestSkipInterval(
- @Nullable Granularity configuredSegmentGranularity,
- DateTime latestDataTimestamp,
- Period skipOffsetFromLatest
+ private static String describeSkipReason(
+ Interval skipInterval,
+ Period skipOffset,
+ List<Interval> configuredSkipIntervals,
+ List<Interval> lockedIntervals
)
{
- if (configuredSegmentGranularity == null) {
- return new Interval(skipOffsetFromLatest, latestDataTimestamp);
- } else {
- DateTime skipFromLastest = new DateTime(latestDataTimestamp,
latestDataTimestamp.getZone()).minus(skipOffsetFromLatest);
- DateTime skipOffsetBucketToSegmentGranularity =
configuredSegmentGranularity.bucketStart(skipFromLastest);
- return new Interval(skipOffsetBucketToSegmentGranularity,
latestDataTimestamp);
+ final StringBuilder reason = new StringBuilder(
+ StringUtils.format("interval[%s] overlaps one of the skip sources:
skipOffsetFromLatest[%s]", skipInterval, skipOffset)
+ );
+ if (!configuredSkipIntervals.isEmpty()) {
Review Comment:
Appending all configured skip intervals to the reason will make it very
verbose and also difficult to identify which exact interval overlapped with the
segment due to which it got skipped.
Ideally, compaction skip reason should be concise so that it may be used as
a metric dimension and perhaps also in the compaction supervisor reports in the
future. It should look something like this:
```
"Interval[%s] locked by another task"
OR
"Interval[%s] skipped by compaction config"
OR
"Skip offset from latest[%s]"
```
##########
server/src/main/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIterator.java:
##########
@@ -437,25 +422,56 @@ List<Interval> findInitialSearchInterval(SegmentTimeline
timeline, List<Interval
.map(segment -> segment.getId().getIntervalEnd())
.max(Comparator.naturalOrder())
.orElseThrow(AssertionError::new);
- searchIntervals.add(new Interval(searchStart, searchEnd));
+ final Interval searchInterval = new Interval(searchStart, searchEnd);
+ final Interval overlappingSkipInterval = allSkipIntervals.stream()
+
.filter(searchInterval::overlaps)
+ .findFirst()
+ .orElse(null);
+ // Guardrail check, this should never happen
Review Comment:
nit: newline for readability
```suggestion
// Guardrail check, this should never happen
```
##########
server/src/main/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIterator.java:
##########
@@ -437,25 +422,56 @@ List<Interval> findInitialSearchInterval(SegmentTimeline
timeline, List<Interval
.map(segment -> segment.getId().getIntervalEnd())
.max(Comparator.naturalOrder())
.orElseThrow(AssertionError::new);
- searchIntervals.add(new Interval(searchStart, searchEnd));
+ final Interval searchInterval = new Interval(searchStart, searchEnd);
Review Comment:
```suggestion
final Interval searchInterval = new Interval(searchStart, searchEnd);
```
##########
server/src/test/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIteratorTest.java:
##########
@@ -145,4 +145,41 @@ public void
testFindInitialSearchIntervalWithMultipleSkipIntervals()
searchIntervals
);
}
+
+ @Test
+ public void testSkipIntervalNotAlignedWithSegmentGranularityIsNotCompacted()
Review Comment:
The test makes sense, thanks!
--
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]
