FrankChen021 commented on code in PR #20027:
URL: https://github.com/apache/druid/pull/20027#discussion_r3789338198
##########
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 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)
+ {
+ if (segmentGranularity == null) {
+ return interval;
}
+ final DateTime alignedStart =
segmentGranularity.bucketStart(interval.getStart());
+ final DateTime endBucketStart =
segmentGranularity.bucketStart(interval.getEnd());
+ final DateTime alignedEnd = endBucketStart.equals(interval.getEnd())
Review Comment:
[P2] Timezone-sensitive boundary comparison expands aligned skips
`DateTime.equals` compares chronology as well as the instant. When a non-UTC
`PeriodGranularity` produces an exactly aligned interval whose end was parsed
in UTC, this check can return false and expand the interval with `bucketEnd`,
causing an adjacent bucket to be skipped. Compare instants with `isEqual` or
millis and add a non-UTC boundary test.
##########
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 skipOffsetFromLatest[%s]",
skipInterval, skipOffset)
Review Comment:
[P3] Skip diagnostics misattribute non-offset skips
`describeSkipReason` always says the interval overlaps
`skipOffsetFromLatest`, even when the interval came only from configured skip
intervals or lock intervals. This makes the new operator-facing diagnostics
misleading. Select the explanation from the actual source interval.
--
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]
