jsancio commented on code in PR #19658:
URL: https://github.com/apache/kafka/pull/19658#discussion_r2082461965
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -1048,14 +1053,18 @@ private void maybeHandleElectionLoss(NomineeState
state, long currentTimeMs) {
}
}
- private int binaryExponentialElectionBackoffMs(int retries) {
+ // visible for testing
+ static int binaryExponentialElectionBackoffMs(int backoffMaxMs, int
retries, Random random) {
if (retries <= 0) {
throw new IllegalArgumentException("Retries " + retries + " should
be larger than zero");
}
- // upper limit exponential co-efficients at 20 to avoid overflow
+ // Takes minimum of the following:
+ // 1. exponential backoff calculation (maxes out at 102.4 seconds)
+ // 2. configurable electionBackoffMaxMs + jitter
+ // The jitter is added to prevent deadlock of elections.
Review Comment:
I don't think deadlock is the correct term. The actors/replicas are not dead
or blocked. I think that livelock is a more accurate description of the issue.
The replicas are changing (i.e. holding election) but they are changing in a
way that they can't make process(i.e. elect a leader).
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -4569,6 +4574,55 @@ public void testObserverFetchWithNoLocalId(boolean
withKip853Rpc) throws Excepti
assertEquals(3, context.log.lastFetchedEpoch());
}
+ @Test
+ public void testExponentialElectionBackoffMs() {
Review Comment:
Did you consider having 3 parametrized tests? On each test invocation you
can create a new mocked `Random`. This should allow you to remove
`clearInvocations` and be sure that your mock objects are behaving correctly.
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -1048,14 +1053,18 @@ private void maybeHandleElectionLoss(NomineeState
state, long currentTimeMs) {
}
}
- private int binaryExponentialElectionBackoffMs(int retries) {
+ // visible for testing
+ static int binaryExponentialElectionBackoffMs(int backoffMaxMs, int
retries, Random random) {
Review Comment:
How about moving this static method to `RaftUtil` and make it public? If you
do this you can also move the test to `RaftUtilTest`.
--
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]
