chia7712 commented on code in PR #15877: URL: https://github.com/apache/kafka/pull/15877#discussion_r1591714910
########## clients/src/test/java/org/apache/kafka/clients/MetadataTest.java: ########## @@ -1323,7 +1323,7 @@ public void testConcurrentUpdateAndFetchForSnapshotAndCluster() throws Interrupt } else { // Thread to read metadata snapshot, once its updated try { if (!atleastMetadataUpdatedOnceLatch.await(5, TimeUnit.MINUTES)) { Review Comment: How about using `assertDoesNotThrow`? For example: ```java assertTrue(assertDoesNotThrow(() -> atleastMetadataUpdatedOnceLatch.await(5, TimeUnit.MINUTES)), "Test had to wait more than 5 minutes, something went wrong."); ``` ########## clients/src/test/java/org/apache/kafka/clients/MetadataTest.java: ########## @@ -1335,7 +1335,7 @@ public void testConcurrentUpdateAndFetchForSnapshotAndCluster() throws Interrupt }); } if (!allThreadsDoneLatch.await(5, TimeUnit.MINUTES)) { Review Comment: How about using `assertTrue`? ```java assertTrue(allThreadsDoneLatch.await(5, TimeUnit.MINUTES), "Test had to wait more than 5 minutes, something went wrong."); ``` ########## clients/src/test/java/org/apache/kafka/common/header/internals/RecordHeadersTest.java: ########## @@ -220,9 +220,7 @@ public void shouldThrowNpeWhenAddingCollectionWithNullHeader() { private int getCount(Headers headers) { int count = 0; - Iterator<Header> headerIterator = headers.iterator(); - while (headerIterator.hasNext()) { - headerIterator.next(); + for (Header ignore : headers) { Review Comment: How about using `toArray`? for example: `headers.toArray().length` ########## clients/src/test/java/org/apache/kafka/test/Microbenchmarks.java: ########## @@ -78,34 +78,30 @@ public static void main(String[] args) throws Exception { final Time time = Time.SYSTEM; final AtomicBoolean done = new AtomicBoolean(false); final Object lock = new Object(); Review Comment: Maybe we should just delete this old class ... ########## clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java: ########## @@ -106,10 +103,9 @@ public void setup() { commitRequestManager = testBuilder.commitRequestManager.orElseThrow(IllegalStateException::new); offsetsRequestManager = testBuilder.offsetsRequestManager; coordinatorRequestManager = testBuilder.coordinatorRequestManager.orElseThrow(IllegalStateException::new); - heartbeatRequestManager = testBuilder.heartbeatRequestManager.orElseThrow(IllegalStateException::new); - memberhipsManager = testBuilder.membershipManager.orElseThrow(IllegalStateException::new); + HeartbeatRequestManager heartbeatRequestManager = testBuilder.heartbeatRequestManager.orElseThrow(IllegalStateException::new); Review Comment: The variables `heartbeatRequestManager` and `membershipManager` are unused. Are they used to test the existence of `heartbeatRequestManager` and `membershipManager`? If so, could we rewrite them by `assertTrue`? For example: `assertTrue(testBuilder.heartbeatRequestManager.isPresent());` -- 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