Re: [PR] KAFKA-20895: Grant pre-vote once the leader has resigned [kafka]

["kevin-wu24 (via GitHub)" <gi...@apache.org>]

kevin-wu24 commented on code in PR #23168:
URL: https://github.com/apache/kafka/pull/23168#discussion_r4273989281


##########
raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java:
##########
@@ -195,6 +197,83 @@ void canElectNewLeaderAfterOldLeaderFailure() {
         });
     }
 
+    @ParameterizedTest
+    @ValueSource(ints = {4, 5, 6, 7, 8, 9})

Review Comment:
   I do not think we should have a simulation test for this. This issue is more of a performance issue, whereas the simulation tests are conceptually more about ensuring the invariants of the KRaft protocol are upheld. Additionally, simulation tests are more expensive to run than the unit tests. For example, this test could fail if you decided to drop some messages, which happens in the real world a lot.
   
   See my other comment for how you can make the unit test model this situation without using a simulation test.



##########
raft/src/main/java/org/apache/kafka/raft/FollowerState.java:
##########
@@ -154,6 +158,14 @@ public void overrideFetchTimeout(long currentTimeMs, long timeoutMs) {
         fetchTimer.reset(timeoutMs);
     }
 
+    /**
+     * Record that the leader of this epoch has resigned, after receiving an `EndQuorumEpoch`
+     * request from it.
+     */
+    public void setLeaderHasResigned() {

Review Comment:
   nit: `setHasLeaderResigned`



##########
raft/src/test/java/org/apache/kafka/raft/FollowerStateTest.java:
##########
@@ -122,6 +122,24 @@ public void testPreVoteAfterSuccessfulFetchFromLeader(boolean isLogUpToDate) {
         assertFalse(state.canGrantVote(ReplicaKey.of(10, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, false));
     }
 
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    public void testPreVoteAfterLeaderResigned(boolean isLogUpToDate) {
+        FollowerState state = newFollowerState(Set.of(1, 2, 3));
+        state.resetFetchTimeoutForSuccessfulFetch(time.milliseconds());
+        state.setLeaderHasResigned();
+
+        assertEquals(isLogUpToDate, state.canGrantVote(ReplicaKey.of(1, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, true));
+        assertEquals(isLogUpToDate, state.canGrantVote(ReplicaKey.of(2, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, true));
+        assertEquals(isLogUpToDate, state.canGrantVote(ReplicaKey.of(3, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, true));
+        assertEquals(isLogUpToDate, state.canGrantVote(ReplicaKey.of(10, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, true));
+
+        assertFalse(state.canGrantVote(ReplicaKey.of(1, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, false));
+        assertFalse(state.canGrantVote(ReplicaKey.of(2, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, false));
+        assertFalse(state.canGrantVote(ReplicaKey.of(3, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, false));
+        assertFalse(state.canGrantVote(ReplicaKey.of(10, ReplicaKey.NO_DIRECTORY_ID), isLogUpToDate, false));
+    }

Review Comment:
   In my opinion, a better test would have the local follower node start as rejecting preVotes from a remote node whose log is up to date because `hasFetchedFromLeader` is true. Then, the follower is delivered an `EndQuorumEpochRequest` to model the leader resigning. Finally, this test should show that a preVote is now granted by the local follower. 
   
   Writing a unit test like this also makes it so we do not need the simulation test below IMO. It would go in `KafkaRaftClientPreVoteTest`.
   
   What do you think?



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org