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
