[GitHub] [kafka] dajac commented on a diff in pull request #13868: MINOR: Close ReplicaManager correctly in ReplicaManagerTest

["dajac (via GitHub)" <gi...@apache.org>]

dajac commented on code in PR #13868:
URL: https://github.com/apache/kafka/pull/13868#discussion_r1235012466


##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -343,6 +348,7 @@ class ReplicaManagerTest {
 
       assertTrue(appendResult.hasFired)
     } finally {
+      // shutdown the replica manager upon test completion

Review Comment:
   This comment is only there in certain cases. Should we just remove it everywhere? The intent is pretty obvious here in my opinion.



##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -4935,12 +4991,17 @@ class ReplicaManagerTest {
       topicPartition, leaderEpoch + leaderEpochIncrement, followerBrokerId, leaderBrokerId, countDownLatch,
       expectTruncation = false, localLogOffset = Some(10), offsetFromLeader = offsetFromLeader, topicId = Some(topicId))
 
-    val responses = replicaManager.describeLogDirs(Set(new TopicPartition(topic, topicPartition)))
-    assertEquals(mockLogMgr.liveLogDirs.size, responses.size)
-    responses.foreach { response =>
-      assertEquals(Errors.NONE.code, response.errorCode)
-      assertTrue(response.totalBytes > 0)
-      assertTrue(response.usableBytes >= 0)
+    try {
+      val responses = replicaManager.describeLogDirs(Set(new TopicPartition(topic, topicPartition)))
+      assertEquals(mockLogMgr.liveLogDirs.size, responses.size)
+      responses.foreach { response =>
+        assertEquals(Errors.NONE.code, response.errorCode)
+        assertTrue(response.totalBytes > 0)
+        assertTrue(response.usableBytes >= 0)
+      }
+    } finally {
+      // shutdown the replica manager upon test completion
+      replicaManager.shutdown(checkpointHW = false)

Review Comment:
   Now that we use `checkpointHW = false` everywhere, do we still have at least one test with `checkpointHW = true` to ensure that it works?



##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -84,7 +84,7 @@ import scala.jdk.CollectionConverters._
 class ReplicaManagerTest {
 
   val topic = "test-topic"
-  val topicId = Uuid.randomUuid()
+  val topicId: Uuid = Uuid.randomUuid()

Review Comment:
   It is surprising that we only have to do this for `topicId`.  Instead of doing this, I wonder if we should make all the attributes private. What do you think?



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org