FrankChen021 commented on code in PR #19922:
URL: https://github.com/apache/druid/pull/19922#discussion_r3739910999
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilterTest.java:
##########
@@ -81,8 +81,8 @@ public void testTaskNotFound()
catch (WebApplicationException e) {
expected = e;
}
- Assert.assertNotNull(expected);
- Assert.assertEquals(expected.getResponse().getStatus(),
Response.Status.NOT_FOUND.getStatusCode());
+ Assertions.assertNotNull(expected);
+ Assertions.assertEquals(expected.getResponse().getStatus(),
Response.Status.NOT_FOUND.getStatusCode());
Review Comment:
Addressed in the existing worktree: the assertion now uses the conventional
expected/actual order,
`Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
expected.getResponse().getStatus())`. This thread is outdated against the
current diff, and the focused TaskResourceFilterTest passes.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilterTest.java:
##########
@@ -150,8 +150,8 @@ public void testSupervisorNotFound()
expected = e;
}
- Assert.assertNotNull(expected);
- Assert.assertEquals(expected.getResponse().getStatus(),
Response.Status.NOT_FOUND.getStatusCode());
+ Assertions.assertNotNull(expected);
+ Assertions.assertEquals(expected.getResponse().getStatus(),
Response.Status.NOT_FOUND.getStatusCode());
Review Comment:
Addressed in the existing worktree: the assertion now uses the conventional
expected/actual order,
`Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
expected.getResponse().getStatus())`. This thread is outdated against the
current diff, and the focused SupervisorResourceFilterTest passes.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/WorkerTaskRunnerQueryAdpaterTest.java:
##########
@@ -118,21 +118,24 @@ public void testDisableWorkerWhenWorkerRaisesError()
throws Exception
try {
workerTaskRunnerQueryAdapter.disableWorker("worker-host1");
- Assert.fail("Should raise RE exception!");
+ Assertions.fail("Should raise RE exception!");
}
catch (RE re) {
Review Comment:
Addressed both occurrences in the existing worktree by replacing the
try/catch plus `fail()` pattern with `Assertions.assertThrows(RE.class, ...)`
for `disableWorker` and `enableWorker`. The focused
WorkerTaskRunnerQueryAdpaterTest passes.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ThreadingTaskRunnerTest.java:
##########
@@ -85,8 +85,8 @@ public TaskStatus runTask(TaskToolbox toolbox)
});
TaskStatus status = statusFuture.get();
Review Comment:
Addressed in the existing worktree by declaring the non-reassigned `status`
local as `final`. This thread is outdated against the current diff, and the
focused ThreadingTaskRunnerTest passes.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/duty/UnusedSegmentsKillerTest.java:
##########
@@ -61,7 +62,6 @@
public class UnusedSegmentsKillerTest
{
- @Rule
public TaskActionTestKit taskActionTestKit = new TaskActionTestKit();
Review Comment:
Addressed in the existing worktree by making `taskActionTestKit` private and
final. This thread is outdated against the current diff, and the focused
UnusedSegmentsKillerTest passes.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java:
##########
@@ -1043,8 +1035,8 @@
TaskStatusResponse.class
);
TaskStatusPlus tsp = taskStatusResponse1.getStatus();
- Assert.assertEquals(tsp.getStatusCode(), tsp.getStatus());
- Assert.assertEquals(
+ Assertions.assertEquals(tsp.getStatusCode(), tsp.getStatus());
Review Comment:
Addressed in the existing worktree by removing the redundant deprecated
`TaskStatusPlus.getStatus()` assertion; the status object remains used in the
response equality check. The focused OverlordResourceTest passes.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -55,26 +55,26 @@
null
);
- Assert.assertFalse(config.getEnableTaskAutoScaler());
- Assert.assertEquals(30000, config.getLagCollectionIntervalMillis());
- Assert.assertEquals(600000, config.getLagCollectionRangeMillis());
- Assert.assertEquals(300000, config.getScaleActionStartDelayMillis());
- Assert.assertEquals(60000, config.getScaleActionPeriodMillis());
- Assert.assertEquals(6000000, config.getScaleOutThreshold());
- Assert.assertEquals(1000000, config.getScaleInThreshold());
- Assert.assertEquals(0.3, config.getTriggerScaleOutFractionThreshold(),
0.00001);
- Assert.assertEquals(0.9, config.getTriggerScaleInFractionThreshold(),
0.00001);
- Assert.assertEquals(1, config.getScaleInStep());
- Assert.assertEquals(2, config.getScaleOutStep());
- Assert.assertEquals(600000,
config.getMinTriggerScaleActionFrequencyMillis());
+ Assertions.assertFalse(config.getEnableTaskAutoScaler());
+ Assertions.assertEquals(30000, config.getLagCollectionIntervalMillis());
+ Assertions.assertEquals(600000, config.getLagCollectionRangeMillis());
+ Assertions.assertEquals(300000, config.getScaleActionStartDelayMillis());
+ Assertions.assertEquals(60000, config.getScaleActionPeriodMillis());
+ Assertions.assertEquals(6000000, config.getScaleOutThreshold());
+ Assertions.assertEquals(1000000, config.getScaleInThreshold());
+ Assertions.assertEquals(0.3, config.getTriggerScaleOutFractionThreshold(),
0.00001);
+ Assertions.assertEquals(0.9, config.getTriggerScaleInFractionThreshold(),
0.00001);
+ Assertions.assertEquals(1, config.getScaleInStep());
+ Assertions.assertEquals(2, config.getScaleOutStep());
+ Assertions.assertEquals(600000,
config.getMinTriggerScaleActionFrequencyMillis());
Review Comment:
Retained intentionally. This assertion verifies the deprecated field’s
legacy default value, which is part of the backward-compatibility contract for
`LagBasedAutoScalerConfig`; removing it would reduce coverage. The
compatibility test class is scoped with `@SuppressWarnings("deprecation")` and
all 8 tests pass.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -331,9 +331,9 @@
null,
null
);
- Assert.assertEquals(60000L,
baseOnly.getMinTriggerScaleActionFrequencyMillis());
- Assert.assertEquals(Duration.millis(60000), baseOnly.getMinScaleUpDelay());
- Assert.assertEquals(Duration.millis(60000),
baseOnly.getMinScaleDownDelay());
+ Assertions.assertEquals(60000L,
baseOnly.getMinTriggerScaleActionFrequencyMillis());
Review Comment:
Retained intentionally. This assertion verifies the deprecated field when
the legacy constructor path supplies the base value and the new directional
delays fall back to it; removing it would reduce backward-compatibility
coverage. The compatibility test class is scoped with
`@SuppressWarnings("deprecation")` and all 8 tests pass.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -424,18 +424,18 @@
// both getMinScaleUpDelay() and getMinScaleDownDelay() should fall back
to it.
String json =
"{\"taskCountMax\":10,\"taskCountMin\":1,\"minTriggerScaleActionFrequencyMillis\":45000}";
LagBasedAutoScalerConfig config = OBJECT_MAPPER.readValue(json,
LagBasedAutoScalerConfig.class);
- Assert.assertEquals(45000L,
config.getMinTriggerScaleActionFrequencyMillis());
- Assert.assertEquals(Duration.millis(45000), config.getMinScaleUpDelay());
- Assert.assertEquals(Duration.millis(45000), config.getMinScaleDownDelay());
+ Assertions.assertEquals(45000L,
config.getMinTriggerScaleActionFrequencyMillis());
Review Comment:
Retained intentionally. This assertion verifies deserialization of legacy
JSON containing only `minTriggerScaleActionFrequencyMillis` and its fallback
behavior; removing it would reduce backward-compatibility coverage. The
compatibility test class is scoped with `@SuppressWarnings("deprecation")` and
all 8 tests pass.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -451,9 +451,9 @@
"{\"taskCountMax\":10,\"taskCountMin\":1}",
LagBasedAutoScalerConfig.class
);
- Assert.assertEquals(defaultMinTriggerMillis,
config.getMinTriggerScaleActionFrequencyMillis());
- Assert.assertEquals(Duration.millis(defaultMinTriggerMillis),
config.getMinScaleUpDelay());
- Assert.assertEquals(Duration.millis(defaultMinTriggerMillis),
config.getMinScaleDownDelay());
+ Assertions.assertEquals(defaultMinTriggerMillis,
config.getMinTriggerScaleActionFrequencyMillis());
Review Comment:
Retained intentionally. This assertion verifies the deprecated field’s
default while new directional delay fields are present, covering the
forward-compatibility migration case. Removing it would reduce compatibility
coverage. The compatibility test class is scoped with
`@SuppressWarnings("deprecation")` and all 8 tests pass.
--
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]
