kfaraz commented on code in PR #16334:
URL: https://github.com/apache/druid/pull/16334#discussion_r1582204502
##########
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSupervisorSpecTest.java:
##########
@@ -988,7 +1007,8 @@ public void testSeekableStreamSupervisorSpecWithScaleIn()
throws InterruptedExce
Thread.sleep(1000);
int taskCountAfterScaleOut = supervisor.getIoConfig().getTaskCount();
Assert.assertEquals(1, taskCountAfterScaleOut);
-
+ Assert.assertTrue(timesLagStatsMaxCalled == 0);
+ Assert.assertTrue(timesLagStatsSumCalled > 0);
Review Comment:
> This test verifies the override by the config is working fine.
The correct way to verify that would be to have different
`LagAutoScalerConfig`s and different supervisor impls (which return different
values of `LagStats.aggregateForScaling`) and verify that the value of
`taskCountAfterScaleOut` changes based on whether the config/supervisor uses
MAX or SUM or AVERAGE.
(These tests can also be added later and need not block this PR, unless
required for code coverage).
We should not have to mock or spy the `LagStats` class just to check which
method was invoked on it. For this test class, the end result is the task scale
out and that is all we need to verify. We cannot verify every internal method
called when invoking any public interface.
Please revert these test changes, rest of the PR looks okay to me.
--
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]
