kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525749069
##########
server/src/test/java/org/apache/druid/metadata/TestDerbyConnector.java:
##########
@@ -135,5 +136,23 @@ public Supplier<MetadataStorageTablesConfig>
metadataTablesConfigSupplier()
{
return dbTables;
}
+
+ public Integer updateSegmentsTable(String sqlFormat, Object... args)
Review Comment:
Does the return value have to be a boxed integer?
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -166,16 +164,19 @@ public void testKillWithMarkUnused() throws Exception
null
);
-
Assert.assertEquals(ImmutableList.of(newSegment(Intervals.of("2019-02-01/2019-03-01"),
version)), unusedSegments);
+ Assert.assertEquals(ImmutableList.of(segment2), unusedSegments);
Assertions.assertThat(
- getMetadataStorageCoordinator()
- .retrieveUsedSegmentsForInterval(DATA_SOURCE,
Intervals.of("2019/2020"), Segments.ONLY_VISIBLE)
- ).containsExactlyInAnyOrder(
- newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
- newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
- );
+ getMetadataStorageCoordinator().retrieveUsedSegmentsForInterval(
+ DATA_SOURCE,
+ Intervals.of("2019/2020"),
+ Segments.ONLY_VISIBLE
+ )
+ ).containsExactlyInAnyOrder(segment1, segment4);
- Assert.assertEquals(new KillTaskReport.Stats(1, 2, 1), getReportedStats());
Review Comment:
Doesn't seem like it needs to be broken up across multiple lines. Seems
readable as well as compact the way it currently is. (same comment in a couple
other places)
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -1197,4 +1173,13 @@ private static DataSegment newSegment(Interval interval,
String version, Map<Str
10L
);
}
+
+ private void updateUsedStatusLastUpdated(DataSegment segment, DateTime
newValue)
Review Comment:
Only place I can think is `TestDerbyConnector` itself, or a wrapper around
it.
I would prefer a wrapper as `TestDerbyConnector` itself is completely
agnostic to any column or table names and it is clean that way.
If you do end up writing a wrapper, may be the new `updateSegmentsTable`
should also live there then.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -55,105 +55,103 @@ public class KillUnusedSegmentsTaskTest extends
IngestionTestBase
private TestTaskRunner taskRunner;
+ private DataSegment segment1;
+ private DataSegment segment2;
+ private DataSegment segment3;
+ private DataSegment segment4;
+
@Before
public void setup()
{
taskRunner = new TestTaskRunner();
+
+ final String version = DateTimes.nowUtc().toString();
+ segment1 = newSegment(Intervals.of("2019-01-01/2019-02-01"), version);
+ segment2 = newSegment(Intervals.of("2019-02-01/2019-03-01"), version);
+ segment3 = newSegment(Intervals.of("2019-03-01/2019-04-01"), version);
+ segment4 = newSegment(Intervals.of("2019-04-01/2019-05-01"), version);
}
@Test
public void testKill() throws Exception
{
- final String version = DateTimes.nowUtc().toString();
- final Set<DataSegment> segments = ImmutableSet.of(
- newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
- newSegment(Intervals.of("2019-02-01/2019-03-01"), version),
- newSegment(Intervals.of("2019-03-01/2019-04-01"), version),
- newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
- );
+ final Set<DataSegment> segments = ImmutableSet.of(segment1, segment2,
segment3, segment4);
final Set<DataSegment> announced =
getMetadataStorageCoordinator().commitSegments(segments);
-
Assert.assertEquals(segments, announced);
Assert.assertTrue(
getSegmentsMetadataManager().markSegmentAsUnused(
- newSegment(Intervals.of("2019-02-01/2019-03-01"), version).getId()
+ segment2.getId()
)
);
Assert.assertTrue(
getSegmentsMetadataManager().markSegmentAsUnused(
- newSegment(Intervals.of("2019-03-01/2019-04-01"), version).getId()
+ segment3.getId()
)
);
- final KillUnusedSegmentsTask task =
- new KillUnusedSegmentsTask(
- null,
- DATA_SOURCE,
- Intervals.of("2019-03-01/2019-04-01"),
- null,
- null,
- false,
- null,
- null,
- null
- );
+ final KillUnusedSegmentsTask task = new KillUnusedSegmentsTask(
+ null,
+ DATA_SOURCE,
+ Intervals.of("2019-03-01/2019-04-01"),
+ null,
+ null,
+ false,
+ null,
+ null,
+ null
+ );
Assert.assertEquals(TaskState.SUCCESS,
taskRunner.run(task).get().getStatusCode());
- final List<DataSegment> unusedSegments =
getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
+ final List<DataSegment> observedUnusedSegments =
+ getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
DATA_SOURCE,
Intervals.of("2019/2020"),
null,
null,
null
);
Review Comment:
indentation is off.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -708,66 +695,71 @@ public void
testKillMultipleUnusedSegmentsWithDifferentMaxUsedStatusLastUpdatedT
)
);
- final DateTime maxUsedStatusLastUpdatedTime2 = DateTimes.nowUtc();
-
+ final DateTime lastUpdatedTime2 = DateTimes.nowUtc();
+ updateUsedStatusLastUpdated(segment2, lastUpdatedTime2);
+ updateUsedStatusLastUpdated(segment3, lastUpdatedTime2);
final List<Interval> segmentIntervals = segments.stream()
.map(DataSegment::getInterval)
.collect(Collectors.toList());
final Interval umbrellaInterval =
JodaUtils.umbrellaInterval(segmentIntervals);
-
- final KillUnusedSegmentsTask task1 =
- new KillUnusedSegmentsTask(
- null,
- DATA_SOURCE,
- umbrellaInterval,
- null,
- null,
- false,
- 1,
- 10,
- maxUsedStatusLastUpdatedTime1
- );
+ final KillUnusedSegmentsTask task1 = new KillUnusedSegmentsTask(
+ null,
+ DATA_SOURCE,
+ umbrellaInterval,
+ null,
+ null,
+ false,
+ 1,
+ 10,
+ lastUpdatedTime1
+ );
Assert.assertEquals(TaskState.SUCCESS,
taskRunner.run(task1).get().getStatusCode());
- final List<DataSegment> unusedSegments =
+ final List<DataSegment> observedUnusedSegments1 =
getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
DATA_SOURCE,
umbrellaInterval,
null,
null
- );
-
- Assert.assertEquals(ImmutableList.of(segment2, segment3), unusedSegments);
- Assert.assertEquals(new KillTaskReport.Stats(2, 3, 0), getReportedStats());
-
- final KillUnusedSegmentsTask task2 =
- new KillUnusedSegmentsTask(
- null,
- DATA_SOURCE,
- umbrellaInterval,
- null,
- null,
- false,
- 1,
- 10,
- maxUsedStatusLastUpdatedTime2
);
- Assert.assertEquals(TaskState.SUCCESS,
taskRunner.run(task2).get().getStatusCode());
+ Assert.assertEquals(ImmutableList.of(segment2, segment3),
observedUnusedSegments1);
+ Assert.assertEquals(
+ new KillTaskReport.Stats(2, 3, 0),
+ getReportedStats()
+ );
- final List<DataSegment> unusedSegments2 =
getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
+ final KillUnusedSegmentsTask task2 = new KillUnusedSegmentsTask(
Review Comment:
Should we create a builder for this Task type inside this class?
Seems like many invocations use a lot of nulls.
##########
server/src/test/java/org/apache/druid/metadata/TestDerbyConnector.java:
##########
@@ -135,5 +136,23 @@ public Supplier<MetadataStorageTablesConfig>
metadataTablesConfigSupplier()
{
return dbTables;
}
+
+ public Integer updateSegmentsTable(String sqlFormat, Object... args)
Review Comment:
The javadoc of this method should mention that the `sqlFormat` should have a
`%s` placeholder for the table name and a `?` for each of the query args.
--
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]
