gaurav-narula commented on code in PR #17434:
URL: https://github.com/apache/kafka/pull/17434#discussion_r1794075725
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientReconfigTest.java:
##########
@@ -2163,6 +2165,8 @@ void testFollowerSendsUpdateVoterWhenDifferent() throws
Exception {
context.pollUntilRequest();
RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest();
context.assertFetchRequestData(fetchRequest, epoch, 0L, 0);
+ context.time.sleep(context.fetchTimeoutMs - 1);
+ assertNotEquals(OptionalLong.of(0L),
context.messageQueue.lastPollTimeoutMs());
Review Comment:
> Okay. I have a couple of comments.
>
> Calling `context.time.sleep()` without calling `context.client.poll()`
(directly or indirectly doesn't do anything). You can remove the call to
`sleep` and the test will still be correct.
Thanks for pointing that out. I've removed the redundant call to sleep.
> Let's explain what this check is doing as it is very subtle. This is my
understanding.
>
> ```
> /*
> * After more than 3 fetch timeouts the update voter period timer should
have expired. Check that the update
> * voter period timer doesn't remain at zero (0) and cause the message
queue to get called without a timeout
> * (0 timeout).
> */
> ```
>
> What do you think?
That sounds good. I've modified the last line ever so slightly as `without a
timeout` may give an impression that it blocks indefinitely. It reads:
```
// after more than 3 fetch timeouts the update voter period timer should
have expired.
// check that the update voter period timer doesn't remain at zero (0) and
cause the message queue to get
// called with a zero (0) timeout and result in a busy-loop.
```
Hope that's okay
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
