You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "abhishekrb19 (via GitHub)" <gi...@apache.org> on 2024/03/15 03:55:30 UTC
[PR] Refactor tests and code clean up (druid)
abhishekrb19 opened a new pull request, #16129:
URL: https://github.com/apache/druid/pull/16129
Summary of changes in this patch by file:
#### Changes in `TestDerbyConnector`:
- Introduce a helper method `updateSegmentsTable()` that tests can use to execute `UPDATE`, `INSERT`, or DDL commands like `ALTER TABLE`.
- Clean up references in tests to use the `updateSegmentsTable()` method where applicable. For example, see usages in `SqlSegmentsMetadataManagerTest`, `IndexerSQLMetadataStorageCoordinatorTest`, `SQLMetadataConnectorTest`.
#### Changes in `KillUnusedSegmentsTaskTest`:
- Move common segment creation to `setup()` method.
- Replace usages of `Thread.sleep()` with calls to the new `updateSegmentsTable()` method.
- Miscellaneous indentation adjustments based on review feedback from https://github.com/apache/druid/pull/15994.
#### Change in `IndexerSQLMetadataStorageCoordinator`:
- Refactor some code in favor of the `verifySegmentsToCommit()` method.
This PR has:
- [x] been self-reviewed.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1526425247
##########
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:
Added javadocs and removed the boxed type
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525736033
##########
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:
This function is actually copy-pasted and used in 4 different test classes. Maybe we can find a common place we can put this in?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1526435461
##########
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:
Yeah, it doesn't seem like much when looking at this line, but with the IDE providing parameter hints, things seem longer than they appear in a split screen setting. So I often end up scrolling right through the screen and the line break helps with that.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525755356
##########
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 of 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.
**Edit:**
The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated(segmentId, date);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525760341
##########
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:
Edit: The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated(segmentId, date);
```
##########
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.
Edit: The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated(segmentId, date);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1526427975
##########
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:
I like this suggestion. I went ahead with the wrapper implementation and updated usages!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 merged PR #16129:
URL: https://github.com/apache/druid/pull/16129
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525755356
##########
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 of 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.
**Edit:**
The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` may be able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated(segmentId, date);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525755356
##########
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.
**Edit:**
The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated(segmentId, date);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525760341
##########
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:
Edit: The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated(segmentId, date);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525736033
##########
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:
This function is actually copy-pasted and used in 4 different test classes. Maybe we can find a common place we can put this in for reusability?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525760341
##########
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:
Edit: The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```java
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated("UPDATE %s SET column=value");
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1525760341
##########
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:
Edit: The wrapper class can be invoked via `TestDerbyConnector` itself so that any test using `TestDerbyConnector` maybe able to use it without having to explicitly declare the wrapper. So something like the following.
```
derbyConnector.segments().update("ALTER TABLE ....");
derbyConnector.segments().updateUsedStatusLastUpdated("UPDATE %s SET column=value");
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor tests and code clean up (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #16129:
URL: https://github.com/apache/druid/pull/16129#discussion_r1526425947
##########
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:
Good suggestion 👍 Refactored to use a builder class in this test.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org