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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
