dajac commented on a change in pull request #9573:
URL: https://github.com/apache/kafka/pull/9573#discussion_r519620665
##########
File path: core/src/test/scala/unit/kafka/server/IsrExpirationTest.scala
##########
@@ -62,14 +64,16 @@ class IsrExpirationTest {
EasyMock.replay(logManager)
alterIsrManager = TestUtils.createAlterIsrManager()
+ quotaManager = QuotaFactory.instantiate(configs.head, metrics, time, "")
replicaManager = new ReplicaManager(configs.head, metrics, time, null,
null, logManager, new AtomicBoolean(false),
- QuotaFactory.instantiate(configs.head, metrics, time, ""), new
BrokerTopicStats, new MetadataCache(configs.head.brokerId),
+ quotaManager, new BrokerTopicStats, new
MetadataCache(configs.head.brokerId),
new LogDirFailureChannel(configs.head.logDirs.size), alterIsrManager)
}
@After
def tearDown(): Unit = {
replicaManager.shutdown(false)
+ quotaManager.shutdown()
Review comment:
nit: We should check that `quotaManager` is not null. We can also do it
for `replicaManager` above.
##########
File path: core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
##########
@@ -1506,7 +1508,7 @@ class ReplicaManagerTest {
purgatoryName = "ElectLeader", timer, reaperEnabled = false)
// Mock network client to show leader offset of 5
- val quota = QuotaFactory.instantiate(config, metrics, time, "")
+ val quota = quotaManager
Review comment:
nit: Could we get rid of `quota`?
##########
File path: core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
##########
@@ -79,22 +81,24 @@ class ReplicaManagerTest {
EasyMock.expect(kafkaZkClient.getEntityConfigs(EasyMock.anyString(),
EasyMock.anyString())).andReturn(new Properties()).anyTimes()
EasyMock.replay(kafkaZkClient)
+ val props = TestUtils.createBrokerConfig(1, TestUtils.MockZkConnect)
+ config = KafkaConfig.fromProps(props)
alterIsrManager = EasyMock.createMock(classOf[AlterIsrManager])
+ quotaManager = QuotaFactory.instantiate(config, metrics, time, "")
}
@After
def tearDown(): Unit = {
TestUtils.clearYammerMetrics()
+ quotaManager.shutdown()
Review comment:
Should we check that `quotaManager` is not null?
##########
File path:
core/src/test/scala/unit/kafka/server/epoch/OffsetsForLeaderEpochTest.scala
##########
@@ -112,4 +120,11 @@ class OffsetsForLeaderEpochTest {
//Then
assertEquals(new EpochEndOffset(Errors.UNKNOWN_TOPIC_OR_PARTITION,
UNDEFINED_EPOCH, UNDEFINED_EPOCH_OFFSET), response(tp))
}
+
+ @After
+ def tearDown(): Unit = {
+ replicaManager.shutdown(checkpointHW = false)
+ quotaManager.shutdown()
Review comment:
We should check non null in both cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
