Re: [PR] KAFKA-16286; Notify listener of latest leader and epoch [kafka]
jsancio merged PR #15397: URL: https://github.com/apache/kafka/pull/15397 -- 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
Re: [PR] KAFKA-16286; Notify listener of latest leader and epoch [kafka]
jsancio commented on PR #15397: URL: https://github.com/apache/kafka/pull/15397#issuecomment-1957122692 All of the `raft:test` tests passed. There seem to be some unrelated failures. Running the tests again. -- 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
Re: [PR] KAFKA-16286; Notify listener of latest leader and epoch [kafka]
jsancio commented on PR #15397: URL: https://github.com/apache/kafka/pull/15397#issuecomment-1955046620 > Is there any chance we could fire leader change more than once on a transition? (I guess this might be allowable behavior?) Thanks. I updated the KRaft tests to check that there no duplicate notifications. -- 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
Re: [PR] KAFKA-16286; Notify listener of latest leader and epoch [kafka]
jsancio commented on code in PR #15397:
URL: https://github.com/apache/kafka/pull/15397#discussion_r1496389863
##
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##
@@ -2958,6 +2958,59 @@ public void
testHandleCommitCallbackFiresInCandidateState() throws Exception {
assertEquals(OptionalInt.empty(),
secondListener.currentClaimedEpoch());
}
+@Test
+public void testHandleLeaderChangeFiresAfterUnattachedRegistration()
throws Exception {
+int localId = 0;
+int otherNodeId = 1;
+int epoch = 7;
+Set voters = Utils.mkSet(localId, otherNodeId);
+
+RaftClientTestContext context = new
RaftClientTestContext.Builder(localId, voters)
+.withUnknownLeader(epoch)
+.build();
+
+// Register another listener and verify that it is notified of latest
epoch
+RaftClientTestContext.MockListener secondListener = new
RaftClientTestContext.MockListener(
+OptionalInt.of(localId)
+);
+context.client.register(secondListener);
+context.client.poll();
+
+// Expected leader change notification
+LeaderAndEpoch expectedLeaderAndEpoch = new
LeaderAndEpoch(OptionalInt.empty(), epoch);
+assertEquals(expectedLeaderAndEpoch,
secondListener.currentLeaderAndEpoch());
+
+// Transition to follower and observer leader change
+context.deliverRequest(context.beginEpochRequest(epoch, otherNodeId));
+context.pollUntilResponse();
+
+// Expected leader change notification
+expectedLeaderAndEpoch = new
LeaderAndEpoch(OptionalInt.of(otherNodeId), epoch);
+assertEquals(expectedLeaderAndEpoch,
secondListener.currentLeaderAndEpoch());
+}
+
+@Test
+public void testHandleLeaderChangeFiresAfterFollowerRegistration() throws
Exception {
+int localId = 0;
+int otherNodeId = 1;
+int epoch = 7;
+Set voters = Utils.mkSet(localId, otherNodeId);
+
+RaftClientTestContext context = new
RaftClientTestContext.Builder(localId, voters)
+.withElectedLeader(epoch, otherNodeId)
+.build();
+
+// Register another listener and verify that it is notified of latest
epoch
+RaftClientTestContext.MockListener secondListener = new
RaftClientTestContext.MockListener(
+OptionalInt.of(localId)
+);
+context.client.register(secondListener);
+context.client.poll();
+
+LeaderAndEpoch expectedLeaderAndEpoch = new
LeaderAndEpoch(OptionalInt.of(otherNodeId), epoch);
+assertEquals(expectedLeaderAndEpoch,
secondListener.currentLeaderAndEpoch());
+}
Review Comment:
Both of these tests fail against trunk.
--
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
[PR] KAFKA-16286; Notify listener of latest leader and epoch [kafka]
jsancio opened a new pull request, #15397: URL: https://github.com/apache/kafka/pull/15397 KRaft was only notifying listeners of the latest leader and epoch when the replica transition to a new state. This can result in the listener never getting notified if the registration happened after it had become a follower. This problem doesn't exists for the active leader because the KRaft implementation attempts to notified the listener of the latest leader and epoch when the replica is the active leader. This issue is fixed by notifying the listeners of the latest leader and epoch after processing the listener registration request. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- 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
