FrankChen021 commented on code in PR #19876:
URL: https://github.com/apache/druid/pull/19876#discussion_r3714582907
##########
extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsDataSegmentKillerTest.java:
##########
@@ -354,87 +353,86 @@ public String getStorageDirectory()
Path taskDir = new Path(
testRoot.getAbsolutePath() + Path.SEPARATOR + onDiskRelativePath +
Path.SEPARATOR + "leaf"
);
- Assert.assertTrue(fs.mkdirs(taskDir.getParent()));
+ Assertions.assertTrue(fs.mkdirs(taskDir.getParent()));
fs.createNewFile(taskDir);
killer.killRecursively(relativePathWithColons);
- Assert.assertFalse(fs.exists(new Path(testRoot.getAbsolutePath() +
Path.SEPARATOR + onDiskRelativePath)));
- Assert.assertTrue(fs.exists(batchRoot));
- Assert.assertTrue(fs.delete(batchRoot, true));
+ Assertions.assertFalse(fs.exists(new Path(testRoot.getAbsolutePath() +
Path.SEPARATOR + onDiskRelativePath)));
+ Assertions.assertTrue(fs.exists(batchRoot));
+ Assertions.assertTrue(fs.delete(batchRoot, true));
}
finally {
fs.delete(new Path(testRoot.getAbsolutePath()), true);
}
}
@Test
- public void testKillNonZipSegment() throws Exception
+ public void testKillNonZipSegment()
{
- Configuration config = new Configuration();
- HdfsDataSegmentKiller killer = new HdfsDataSegmentKiller(
- config,
- new HdfsDataSegmentPusherConfig()
- {
- @Override
- public String getStorageDirectory()
+ Throwable exception = assertThrows(SegmentLoadingException.class, () -> {
+ Configuration config = new Configuration();
+ HdfsDataSegmentKiller killer = new HdfsDataSegmentKiller(
+ config,
+ new HdfsDataSegmentPusherConfig()
{
- return "/tmp";
+ @Override
+ public String getStorageDirectory()
+ {
+ return "/tmp";
+ }
}
- }
- );
-
- expectedException.expect(SegmentLoadingException.class);
- expectedException.expectMessage("Unknown file type");
- killer.kill(getSegmentWithPath(new Path("/xxx/",
"index.beep").toString()));
+ );
+ killer.kill(getSegmentWithPath(new Path("/xxx/",
"index.beep").toString()));
+ });
+ assertTrue(exception.getMessage().contains("Unknown file type"));
}
@Test
- public void testNoStorageDirectory() throws Exception
+ public void testNoStorageDirectory()
{
- Configuration config = new Configuration();
- HdfsDataSegmentKiller killer = new HdfsDataSegmentKiller(
- config,
- new HdfsDataSegmentPusherConfig()
- {
- @Override
- public String getStorageDirectory()
+ Throwable exception = assertThrows(IllegalStateException.class, () -> {
Review Comment:
[P2] Limit the exception assertion to killAll
The `assertThrows` lambda now includes filesystem setup, the successful
`killer.kill(...)` path, its assertions, cleanup, and `killAll()`. If an
earlier operation starts throwing the same `IllegalStateException`, this test
passes early, skips the behavior it was meant to verify, and can leave
`/tmp/dataSourceNew` behind. Keep setup, `kill`, assertions, and cleanup
outside the lambda and wrap only `killer.killAll()`.
##########
extensions-core/hdfs-storage/src/test/java/org/apache/druid/inputsource/hdfs/HdfsInputSourceTest.java:
##########
@@ -95,47 +97,46 @@ public class HdfsInputSourceTest extends
InitializedNullHandlingTest
null
);
- public static class ConstructorTest
+ @Nested
+ public class ConstructorTest
{
- @Rule
- public ExpectedException expectedException = ExpectedException.none();
@Test
public void testConstructorAllowsOnlyDefaultProtocol()
{
- HdfsInputSource.builder()
- .paths(PATH + "*")
- .configuration(CONFIGURATION)
- .inputSourceConfig(DEFAULT_INPUT_SOURCE_CONFIG)
- .build();
-
- expectedException.expect(IllegalArgumentException.class);
- expectedException.expectMessage("Only [hdfs] protocols are allowed");
- HdfsInputSource.builder()
- .paths("file:/foo/bar*")
- .configuration(CONFIGURATION)
- .inputSourceConfig(DEFAULT_INPUT_SOURCE_CONFIG)
- .build();
+ Throwable exception = assertThrows(IllegalArgumentException.class, () ->
{
Review Comment:
[P2] Keep the valid HDFS case outside assertThrows
The lambda first builds a valid `hdfs` input source and then the invalid
`file:` source. If the valid construction regresses and throws the expected
`IllegalArgumentException`, the assertion succeeds before the invalid protocol
is tested. Build the allowed source before `assertThrows` and put only the
rejected `file:` construction inside it.
##########
extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/cache/loading/LoadingCacheTest.java:
##########
@@ -50,42 +46,43 @@ public static Collection<Object[]> inputData()
});
}
- private final LoadingCache loadingCache;
+ private LoadingCache loadingCache;
- public LoadingCacheTest(LoadingCache loadingCache)
+ private void initLoadingCacheTest(LoadingCache loadingCache)
{
this.loadingCache = loadingCache;
- }
-
- @Before
- public void setUp()
- {
- Assert.assertFalse(loadingCache.isClosed());
+ Assertions.assertFalse(loadingCache.isClosed());
loadingCache.putAll(IMMUTABLE_MAP);
}
- @After
+ @AfterEach
Review Comment:
[P3] Close each generated off-heap cache
Each parameterized method now invokes `inputData()` separately and creates a
fresh `OffHeapLoadingCache`, but teardown only calls `invalidateAll()`.
`close()` is what deletes the cache map from the static direct-memory MapDB, so
this migration multiplies retained off-heap maps until JVM shutdown. Close the
current cache in `@AfterEach`.
--
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]
