kfaraz commented on code in PR #19792:
URL: https://github.com/apache/druid/pull/19792#discussion_r3687993190
##########
server/src/main/java/org/apache/druid/server/coordinator/InlineSchemaDataSourceCompactionConfig.java:
##########
@@ -105,6 +108,7 @@ public InlineSchemaDataSourceCompactionConfig(
: inputSegmentSizeBytes;
this.maxRowsPerSegment = maxRowsPerSegment;
this.skipOffsetFromLatest = skipOffsetFromLatest == null ?
DEFAULT_SKIP_OFFSET_FROM_LATEST : skipOffsetFromLatest;
+ this.skipIntervals = skipIntervals == null ? List.of() : skipIntervals;
Review Comment:
Nit: Configs.valueOrDefault() for brevity.
##########
server/src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfig.java:
##########
@@ -80,6 +81,8 @@ public interface DataSourceCompactionConfig
Period getSkipOffsetFromLatest();
+ List<Interval> getSkipIntervals();
Review Comment:
Maybe add a 1-line javadoc clarifying that an interval overlapping any of
the intervals in this list will be skipped from compaction.
##########
server/src/test/java/org/apache/druid/server/coordinator/CatalogDataSourceCompactionConfigTest.java:
##########
@@ -144,6 +151,37 @@ public void testSerde() throws JsonProcessingException
);
}
+ @Test
+ public void testSerdeWithSkip() throws JsonProcessingException
Review Comment:
```suggestion
public void testSerdeWithSkipIntervals() throws JsonProcessingException
```
##########
server/src/test/java/org/apache/druid/server/compaction/NewestSegmentFirstPolicyTest.java:
##########
@@ -534,6 +534,43 @@ public void testWithSkipIntervals()
);
}
+ @Test
+ public void testWithConfiguredSkipIntervals()
+ {
+ // Uses createIterator(), which mirrors the Coordinator duty path
+ // (PriorityBasedCompactionSegmentIterator with no externally supplied
skip intervals), to
+ // verify that config.getSkipIntervals() is honored even when nothing else
carries skip
+ // information into the iterator.
Review Comment:
Some of this info might be captured in the method name itself.
##########
server/src/main/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIterator.java:
##########
@@ -454,43 +458,6 @@ static Interval computeLatestSkipInterval(
}
}
- @VisibleForTesting
- static List<Interval> sortAndAddSkipIntervalFromLatest(
Review Comment:
Thanks for the cleanup!
##########
server/src/test/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIteratorTest.java:
##########
@@ -54,22 +72,86 @@ public void testFilterSkipIntervals()
}
@Test
- public void testAddSkipIntervalFromLatestAndSort()
+ public void testFindInitialSearchInterval()
{
- final List<Interval> expectedIntervals = ImmutableList.of(
- Intervals.of("2018-12-24/2018-12-25"),
- Intervals.of("2018-12-29/2019-01-01")
+ final Iterator<DataSegment> segments =
CreateDataSegments.ofDatasource("test_datasource")
+ .forIntervals(12,
Granularities.HOUR)
+
.startingAt("2018-01-01")
+
.withNumPartitions(1)
+
.eachOfSizeInMb(100)
+ .iterator();
+ final SegmentTimeline timeline = SegmentTimeline.forSegments(segments);
+ final DataSourceCompactionConfig config =
+ InlineSchemaDataSourceCompactionConfig.builder()
+ .forDataSource("test_datasource")
+ .withSkipOffsetFromLatest(new
Period("PT4H"))
+ .build();
+ final DataSourceCompactibleSegmentIterator iterator = new
DataSourceCompactibleSegmentIterator(
+ config,
+ timeline,
+ List.of(),
+ POLICY,
+ FINGERPRINT_MAPPER
);
- /*final List<Interval> fullSkipIntervals =
DataSourceCompactibleSegmentIterator.sortAndAddSkipIntervalFromLatest(
- DateTimes.of("2019-01-01"),
- new Period(72, 0, 0, 0),
- null,
- ImmutableList.of(
- Intervals.of("2018-12-30/2018-12-31"),
- Intervals.of("2018-12-24/2018-12-25")
- )
- );*/
- //Assert.assertEquals(expectedIntervals, fullSkipIntervals);
+ final List<Interval> searchIntervals =
iterator.findInitialSearchInterval(timeline, List.of());
+
+ // Expected: Total interval is 2018-01-01T00:00:00/2018-01-01T12:00:00
+ // Skip interval: 2018-01-01T08:00:00/2018-01-01T12:00:00 (computed from
4h offset)
+ // Search interval should be: [2018-01-01T00:00:00/2018-01-01T08:00:00]
+ Assert.assertEquals(1, searchIntervals.size());
+
Assert.assertEquals(Intervals.of("2018-01-01T00:00:00/2018-01-01T08:00:00"),
searchIntervals.get(0));
+ }
+
+ @Test
+ public void testFindInitialSearchIntervalWithMultipleSkipIntervals()
+ {
+ final Iterator<DataSegment> segments =
CreateDataSegments.ofDatasource("test_datasource")
+ .forIntervals(24,
Granularities.HOUR)
+
.startingAt("2018-01-01")
+
.withNumPartitions(1)
+
.eachOfSizeInMb(100)
+ .iterator();
+ final SegmentTimeline timeline = SegmentTimeline.forSegments(segments);
+ final DataSourceCompactionConfig config =
+ InlineSchemaDataSourceCompactionConfig.builder()
+ .forDataSource("test_datasource")
+ .withSkipOffsetFromLatest(new
Period("PT4H"))
+ .withSkipIntervals(List.of(
+
Intervals.of("2018-01-01T06:00:00/2018-01-01T08:00:00"),
+
Intervals.of("2018-01-01T12:00:00/2018-01-01T14:00:00"),
+
Intervals.of("2018-01-01T18:30:00/2018-01-01T21:00:00")
+ ))
+ .build();
+ final DataSourceCompactibleSegmentIterator iterator = new
DataSourceCompactibleSegmentIterator(
+ config,
+ timeline,
+ List.of(),
+ POLICY,
+ FINGERPRINT_MAPPER
+ );
+
+ final List<Interval> searchIntervals =
iterator.findInitialSearchInterval(timeline, List.of());
+
+ // Expected: Total interval is 2018-01-01T00:00:00/2018-01-02T00:00:00
+ // Skip intervals: 2018-01-01T06:00:00/2018-01-01T08:00:00 (explicit)
+ // 2018-01-01T12:00:00/2018-01-01T14:00:00 (explicit)
+ // 2018-01-01T18:30:00/2018-01-01T21:00:00 (explicit)
+ // 2018-01-01T20:00:00/2018-01-02T00:00:00 (computed from
4h offset)
+ // After condensing: 2018-01-01T06:00:00/2018-01-01T08:00:00
+ // 2018-01-01T12:00:00/2018-01-01T14:00:00
+ // 2018-01-01T18:30:00/2018-01-02T00:00:00 (merged from
overlapping intervals)
+ // Filtered intervals: 2018-01-01T00:00:00/2018-01-01T06:00:00
+ // 2018-01-01T08:00:00/2018-01-01T12:00:00
+ // 2018-01-01T14:00:00/2018-01-01T18:30:00
+ // Actual segments are hourly (aligned to hour boundaries), so the segment
18:00-19:00 overlaps with skip interval starting at 18:30 and is excluded.
+ // Search intervals based on actual segment boundaries:
+ // [2018-01-01T00:00:00/2018-01-01T06:00:00,
+ // 2018-01-01T08:00:00/2018-01-01T12:00:00,
+ // 2018-01-01T14:00:00/2018-01-01T18:00:00]
+ Assert.assertEquals(3, searchIntervals.size());
+
Assert.assertEquals(Intervals.of("2018-01-01T00:00:00/2018-01-01T06:00:00"),
searchIntervals.get(0));
+
Assert.assertEquals(Intervals.of("2018-01-01T08:00:00/2018-01-01T12:00:00"),
searchIntervals.get(1));
+
Assert.assertEquals(Intervals.of("2018-01-01T14:00:00/2018-01-01T18:00:00"),
searchIntervals.get(2));
Review Comment:
May be condense this into a single assertion:
```suggestion
Assert.assertEquals(
List.of(
Intervals.of("2018-01-01T00:00:00/PT6H"),
Intervals.of("2018-01-01T08:00:00/PT4H"),
Intervals.of("2018-01-01T14:00:00/PT4H")
),
searchIntervals
);
```
##########
server/src/test/java/org/apache/druid/server/compaction/DataSourceCompactibleSegmentIteratorTest.java:
##########
@@ -54,22 +72,86 @@ public void testFilterSkipIntervals()
}
@Test
- public void testAddSkipIntervalFromLatestAndSort()
+ public void testFindInitialSearchInterval()
{
- final List<Interval> expectedIntervals = ImmutableList.of(
- Intervals.of("2018-12-24/2018-12-25"),
- Intervals.of("2018-12-29/2019-01-01")
+ final Iterator<DataSegment> segments =
CreateDataSegments.ofDatasource("test_datasource")
+ .forIntervals(12,
Granularities.HOUR)
+
.startingAt("2018-01-01")
+
.withNumPartitions(1)
+
.eachOfSizeInMb(100)
+ .iterator();
+ final SegmentTimeline timeline = SegmentTimeline.forSegments(segments);
+ final DataSourceCompactionConfig config =
+ InlineSchemaDataSourceCompactionConfig.builder()
+ .forDataSource("test_datasource")
+ .withSkipOffsetFromLatest(new
Period("PT4H"))
+ .build();
+ final DataSourceCompactibleSegmentIterator iterator = new
DataSourceCompactibleSegmentIterator(
+ config,
+ timeline,
+ List.of(),
+ POLICY,
+ FINGERPRINT_MAPPER
);
- /*final List<Interval> fullSkipIntervals =
DataSourceCompactibleSegmentIterator.sortAndAddSkipIntervalFromLatest(
- DateTimes.of("2019-01-01"),
- new Period(72, 0, 0, 0),
- null,
- ImmutableList.of(
- Intervals.of("2018-12-30/2018-12-31"),
- Intervals.of("2018-12-24/2018-12-25")
- )
- );*/
- //Assert.assertEquals(expectedIntervals, fullSkipIntervals);
+ final List<Interval> searchIntervals =
iterator.findInitialSearchInterval(timeline, List.of());
+
+ // Expected: Total interval is 2018-01-01T00:00:00/2018-01-01T12:00:00
+ // Skip interval: 2018-01-01T08:00:00/2018-01-01T12:00:00 (computed from
4h offset)
+ // Search interval should be: [2018-01-01T00:00:00/2018-01-01T08:00:00]
+ Assert.assertEquals(1, searchIntervals.size());
+
Assert.assertEquals(Intervals.of("2018-01-01T00:00:00/2018-01-01T08:00:00"),
searchIntervals.get(0));
+ }
+
+ @Test
+ public void testFindInitialSearchIntervalWithMultipleSkipIntervals()
+ {
+ final Iterator<DataSegment> segments =
CreateDataSegments.ofDatasource("test_datasource")
+ .forIntervals(24,
Granularities.HOUR)
+
.startingAt("2018-01-01")
+
.withNumPartitions(1)
+
.eachOfSizeInMb(100)
+ .iterator();
+ final SegmentTimeline timeline = SegmentTimeline.forSegments(segments);
+ final DataSourceCompactionConfig config =
+ InlineSchemaDataSourceCompactionConfig.builder()
+ .forDataSource("test_datasource")
+ .withSkipOffsetFromLatest(new
Period("PT4H"))
+ .withSkipIntervals(List.of(
+
Intervals.of("2018-01-01T06:00:00/2018-01-01T08:00:00"),
+
Intervals.of("2018-01-01T12:00:00/2018-01-01T14:00:00"),
+
Intervals.of("2018-01-01T18:30:00/2018-01-01T21:00:00")
+ ))
+ .build();
+ final DataSourceCompactibleSegmentIterator iterator = new
DataSourceCompactibleSegmentIterator(
+ config,
+ timeline,
+ List.of(),
+ POLICY,
+ FINGERPRINT_MAPPER
+ );
+
+ final List<Interval> searchIntervals =
iterator.findInitialSearchInterval(timeline, List.of());
+
+ // Expected: Total interval is 2018-01-01T00:00:00/2018-01-02T00:00:00
+ // Skip intervals: 2018-01-01T06:00:00/2018-01-01T08:00:00 (explicit)
+ // 2018-01-01T12:00:00/2018-01-01T14:00:00 (explicit)
+ // 2018-01-01T18:30:00/2018-01-01T21:00:00 (explicit)
+ // 2018-01-01T20:00:00/2018-01-02T00:00:00 (computed from
4h offset)
+ // After condensing: 2018-01-01T06:00:00/2018-01-01T08:00:00
+ // 2018-01-01T12:00:00/2018-01-01T14:00:00
+ // 2018-01-01T18:30:00/2018-01-02T00:00:00 (merged from
overlapping intervals)
+ // Filtered intervals: 2018-01-01T00:00:00/2018-01-01T06:00:00
+ // 2018-01-01T08:00:00/2018-01-01T12:00:00
+ // 2018-01-01T14:00:00/2018-01-01T18:30:00
+ // Actual segments are hourly (aligned to hour boundaries), so the segment
18:00-19:00 overlaps with skip interval starting at 18:30 and is excluded.
+ // Search intervals based on actual segment boundaries:
+ // [2018-01-01T00:00:00/2018-01-01T06:00:00,
+ // 2018-01-01T08:00:00/2018-01-01T12:00:00,
+ // 2018-01-01T14:00:00/2018-01-01T18:00:00]
Review Comment:
I suppose this is useful but I wish we could condense this comment somehow.
--
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]
