-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINOR: Various cleanups in clients tests #15877
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimaison nice cleanup!
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just delete this old class ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether anyone uses this. I still think we should check before deleting it.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think they are used to test the existence of the managers here, I would say they were just left unused so we should remove them. Managers are retrieved in this way in many other tests (ex HeartbeatRequestManagerTest), but only when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, removing all those unused might help us remove the suppression ClassDataAbstractionCoupling, worth checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, these can be removed and as @lianetm pointed this allows getting rid of the suppression. Thanks
@@ -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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using assertDoesNotThrow
? For example:
assertTrue(assertDoesNotThrow(() -> atleastMetadataUpdatedOnceLatch.await(5, TimeUnit.MINUTES)),
"Test had to wait more than 5 minutes, something went wrong.");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit unusual to chain assert calls but I'll accept it
@@ -1335,7 +1335,7 @@ public void testConcurrentUpdateAndFetchForSnapshotAndCluster() throws Interrupt | |||
}); | |||
} | |||
if (!allThreadsDoneLatch.await(5, TimeUnit.MINUTES)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using assertTrue
?
assertTrue(allThreadsDoneLatch.await(5, TimeUnit.MINUTES), "Test had to wait more than 5 minutes, something went wrong.");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's even better!
Iterator<Header> headerIterator = headers.iterator(); | ||
while (headerIterator.hasNext()) { | ||
headerIterator.next(); | ||
for (Header ignore : headers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using toArray
? for example: headers.toArray().length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion
consumer.subscribe(Collections.singleton(topic)); | ||
fail("Expected an InvalidGroupIdException"); | ||
} catch (InvalidGroupIdException e) { | ||
// OK, expected | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're cleaning up this, maybe just assertThrows
? (same for the following 3 try
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was able to simplify a few of these. Thanks!
@@ -3232,7 +3231,7 @@ public void testProducerBatchRetriesWhenPartitionLeaderChanges() throws Exceptio | |||
assertTrue(client.hasInFlightRequests()); | |||
client.respond(produceResponse(tp0, -1, Errors.NOT_LEADER_OR_FOLLOWER, 0)); | |||
sender.runOnce(); // receive produce response, batch scheduled for retry | |||
assertTrue(!futureIsProduced.isDone(), "Produce request is yet not done."); | |||
assertFalse(futureIsProduced.isDone(), "Produce request is yet not done."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me this error message is inverted too right? it's stating the desired state, not the failure: if the assert fails, it's because the produce request is done when it shouldn't (not because "is yet not done"). Maybe just "Produce request shouldn't complete yet"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I updated the messages
@@ -3253,13 +3252,13 @@ public void testProducerBatchRetriesWhenPartitionLeaderChanges() throws Exceptio | |||
assertTrue(client.hasInFlightRequests()); | |||
client.respond(produceResponse(tp0, -1, Errors.NOT_LEADER_OR_FOLLOWER, 0)); | |||
sender.runOnce(); // receive produce response, schedule batch for retry. | |||
assertTrue(!futureIsProduced.isDone(), "Produce request is yet not done."); | |||
assertFalse(futureIsProduced.isDone(), "Produce request is yet not done."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -3492,9 +3491,9 @@ private void verifyErrorMessage(ProduceResponse response, String expectedMessage | |||
assertEquals(expectedMessage, e1.getCause().getMessage()); | |||
} | |||
|
|||
class AssertEndTxnRequestMatcher implements MockClient.RequestMatcher { | |||
static class AssertEndTxnRequestMatcher implements MockClient.RequestMatcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems could be private too
for (int i = 0; i < testElements.length; i++) { | ||
assertTrue(multiSet.add(testElements[i])); | ||
for (TestElement testElement : testElements) { | ||
assertTrue(multiSet.add(testElement)); | ||
} | ||
for (int i = 0; i < testElements.length; i++) { | ||
assertFalse(multiSet.add(testElements[i])); | ||
for (TestElement testElement : testElements) { | ||
assertFalse(multiSet.add(testElement)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be simplified to a single loop with the 2 asserts in it? we would achieve the same, validate an elem can be added only if new, for all elems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we can merge both loops
} catch (Throwable e) { | ||
assertEquals(partitionRevoked + singleTopicPartition, e.getCause().getCause().getMessage()); | ||
} | ||
t = assertThrows(Throwable.class, () -> consumer.close(Duration.ofMillis(0))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not introduced by this PR, but here we could be more specific and assertThrows(KafkaException.class, ...)
. Regardless of the type of exception thrown in the rebalance callbacks, a KafkaException
is what's always propagated (legacy and new consumer).
Btw, reviewing this PR I noticed that this KafkaConsumerTest has lots of tests not enabled for the new consumer (with lots of TODOs about it). I created KAFKA-16737 to make sure we address it, enable the tests that apply and clean up the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I pushed an update.
Thanks for raising KAFKA-16737
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the reviews! Test failures are not related, merging to trunk |
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Lianet Magrans <lianetmr@gmail.com>
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Lianet Magrans <lianetmr@gmail.com>
Committer Checklist (excluded from commit message)