KAFKA-18561: Remove withKip853Rpc and replace it with withRaftProtocol #18600

frankvicky · 2025-01-17T16:08:12Z

JIRA: KAFKA-18561
Remove withKip853Rpc and replace it with withRaftProtocol and adjust the corresponding test.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

frankvicky · 2025-01-17T16:09:10Z

Hi @ahuang98
Please take a look when you have a free cycle.
Many thanks 🙇🏼

ijuma · 2025-01-22T02:37:06Z

@jsancio You're ok with this?

github-actions · 2025-01-25T03:06:30Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

github-actions · 2025-01-27T03:08:40Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

github-actions · 2025-01-30T03:07:20Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

ahuang98 · 2025-01-31T16:37:08Z

@ijuma I introduced withRaftProtocol and believe @jsancio is okay with this change. I'll review first

ahuang98

With this change it's more obvious that KafkaRaftClientTest only covers KIP_853_PROTOCOL and lower, I wonder if it would be worth doing one of the following

have KafkaRaftClientTest test the whole range of RaftProtocols
replace KIP_853_PROTOCOL with latestProtocol() which will pick the last enum constant in RaftProtocol, and KafkaRaftClientTest will always test the newest protocol
java doc / rename for KafkaRaftClientTest to indicate this only covers a subset of the raft protocols

ahuang98 · 2025-01-31T16:40:46Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

@@ -2265,4 +2267,17 @@ public ByteBuffer buffer() {
            return data;
        }
    }
+
+    private boolean isSupport853(RaftProtocol raftProtocol) {


you can use RaftClientTestContext.isReconfigSupported()

Thanks.
I don't notice that we have such a helper method.

ahuang98 · 2025-01-31T16:46:31Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

+    private static final String KIP_595_PROTOCOL = "KIP_595_PROTOCOL";
+    private static final String KIP_853_PROTOCOL = "KIP_853_PROTOCOL";


my slight pref would be to remove these and just use the strings directly in the EnumSource annotations

suggestion should also be applied to KafkaRaftClientTest if you agree

ahuang98 · 2025-01-31T16:53:10Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

-    @CsvSource({ "false,false", "false,true", "true,false", "true,true" })
-    public void testLeaderListenerNotified(boolean entireLog, boolean withKip853Rpc) throws Exception {
+    @MethodSource("generateKip853TestMatrix")
+    public void testLeaderListenerNotified(boolean entireLog, RaftProtocol raftProtocol) throws Exception {


I guess another option would be just renaming the second parameter to boolean isReconfigSupported, potentially easier for readers to know at a glance that we're not testing the entire set of raftProtocols (rather than navigating to the method source definition)

(then we would do .withRaftProtocol(isReconfigSupported ? KIP_853_PROTOCOL : KIP_595_PROTOCOL) which I understand is a bit bulkier)

ahuang98 · 2025-01-31T16:55:12Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

-        ReplicaKey voter1 = replicaKey(localId + 1, withKip853Rpc);
-        ReplicaKey voter2 = replicaKey(localId + 2, withKip853Rpc);
-        ReplicaKey observer3 = replicaKey(localId + 3, withKip853Rpc);
+        boolean support853 = isSupport853(raftProtocol);


nit: support853 -> reconfigSupported

was this change applied everywhere? we should remove isSupport853() since it's a dupe of reconfigSupported()

Hi @ahuang98
Sorry for inconvience. Previously, I had a finger issue when rebasing.
Now all comments should be applied. PTAL

there was no inconvenience 😅 sorry to hear about your finger! I missed your update today and will review tomorrow morning

Thank you 😆

ahuang98 · 2025-02-19T17:53:27Z

the easiest way is to test all protocols since the key difference between these protocols is support re-config or not.
WDYT?

I don't disagree it would be easy/fine to test all protocols in KafkaRaftClientTest - just to motivate why it may be excessive though - as you said we're basically testing w/o reconfig and w/ reconfig. Since each protocol is cumulative (e.g. KIP_853_PROTOCOL necessarily includes KIP_595_PROTOCOL, KIP_996_PROTOCOL includes KIP_853_PROTOCOL and KIP_595_PROTOCOL), testing a version before KIP_853 and any one version after KIP_853 should be sufficient.

ahuang98 · 2025-02-19T17:54:25Z

Perhaps we can leave my suggestions in #18600 (review) as out of scope - I'll give another review today and see if @jsancio has time to approve

JIRA: KAFKA-18561 Remove withKip853Rpc and replace with withRaftProtocol and adjust corresponding test.

ahuang98 · 2025-03-05T20:02:57Z

@frankvicky let me know when you need another review, I spot checked some of the comments I left earlier that you thumbs up-ed and it seems they haven't all been addressed yet

ahuang98 · 2025-03-07T20:32:01Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java

+        return Stream.of(
+            Arguments.of(KIP_853_PROTOCOL, true),
+            Arguments.of(KIP_853_PROTOCOL, false),
+            Arguments.of(KIP_853_PROTOCOL, true),


this should be KIP_595

ahuang98 · 2025-03-07T20:41:52Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java

        ReplicaKey follower2 = ReplicaKey.of(followerId2, followerDirectoryId2);

-        RaftClientTestContext.Builder builder = new RaftClientTestContext.Builder(localId, localDirectoryId)
-            .withKip853Rpc(withKip853Rpc);
+        Builder builder = new Builder(localId, localDirectoryId)


Just to reduce verbosity? My preference is fully qualifying the Builder class given that it's an inner class with a pretty generic name but I'm honestly not sure which is better coding practice

It's ok for me.
Full qualification is indeed more readable.

ahuang98 · 2025-03-07T20:59:28Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

@@ -276,16 +281,16 @@ public void testListenerRenotified(boolean withKip853Rpc) throws Exception {
    }

    @ParameterizedTest
-    @ValueSource(booleans = { false, true })
-    public void testLeaderImmediatelySendsSnapshotId(boolean withKip853Rpc) throws Exception {
+    @EnumSource(value = RaftProtocol.class)


we should have probably picked the same strategy for KafkaRaftClientSnapshotTest and KafkaRaftClientTest (either test all protocols for both tests or directly translate to matrix of KIP_595 and KIP_853)

I'm not sure if it's a big enough deal to ask you to standardize though - @jsancio thoughts?

I agree that we should try to be consistent. I prefer if we inject the RaftProtocol directly to the test as it is the most flexible abstraction. And to not inject a boolean that then gets converted to a RaftProtocol.

I am okay testing all possible values of RaftProtocol as long as it is not too much work to support and the tests don't take a lot longer.

ahuang98

Thanks a bunch @frankvicky, I've tagged @jsancio to help review

jsancio

Thanks for this improvement @frankvicky . Looks good to me in general. Here are some suggestions to consider.

jsancio · 2025-03-10T18:17:16Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

@@ -276,16 +281,16 @@ public void testListenerRenotified(boolean withKip853Rpc) throws Exception {
    }

    @ParameterizedTest
-    @ValueSource(booleans = { false, true })
-    public void testLeaderImmediatelySendsSnapshotId(boolean withKip853Rpc) throws Exception {
+    @EnumSource(value = RaftProtocol.class)


I agree that we should try to be consistent. I prefer if we inject the RaftProtocol directly to the test as it is the most flexible abstraction. And to not inject a boolean that then gets converted to a RaftProtocol.

I am okay testing all possible values of RaftProtocol as long as it is not too much work to support and the tests don't take a lot longer.

jsancio · 2025-03-10T18:20:04Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

+import static org.apache.kafka.raft.RaftClientTestContext.Builder;
+import static org.apache.kafka.raft.RaftClientTestContext.MockListener;
+import static org.apache.kafka.raft.RaftClientTestContext.RaftProtocol;


Let's revert this change. It cause a lot of unnecessary line changes to this file.

Ok.
Just for memo: RaftProtocol will be extracted as a single file. I will revert Builder and MockListener

jsancio · 2025-03-10T18:23:25Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java

@@ -92,25 +97,25 @@ public void testNodeDirectoryId() {
    }

    @ParameterizedTest
-    @ValueSource(booleans = { true, false })
-    public void testInitializeSingleMemberQuorum(boolean withKip853Rpc) throws IOException {
+    @EnumSource(value = RaftProtocol.class, names = {"KIP_595_PROTOCOL", "KIP_853_PROTOCOL"})


As @ahuang98 mentioned. Let's try to be consistent between KafkaRaftClientTest and KafkaRaftClientSnapshotTest. Ideally we would test all RaftProtocol but I understand if you want to consider that outside the scope of this PR if it is too difficult to change and make all of these tests pass.

I think it could be done in a follow-up PR.
If you agree, I will open a ticket to trace it.

Please ignore the above comment.
I tried to enable all raft protocols for KafkaRaftClientTest, and all tests passed.
I will include this change in the next commit.

jsancio · 2025-03-10T18:26:43Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java

@@ -516,15 +521,15 @@ public void testEndQuorumEpochRetriesWhileResigned(boolean withKip853Rpc) throws
    }

    @ParameterizedTest
-    @ValueSource(booleans = { true, false })
-    public void testResignWillCompleteFetchPurgatory(boolean withKip853Rpc) throws Exception {
+    @EnumSource(value = RaftClientTestContext.RaftProtocol.class, names = {"KIP_595_PROTOCOL", "KIP_853_PROTOCOL"})


You imported RaftProtocol (import static ....RaftClientTestContext.RaftProtocol) so you should be able to reference RaftProtocol directly like you do in the method parameter. E.g. RaftProtocol raftProtocol.

I am also okay if you want to pull RaftProtocol into its own file. E.g. raft/src/test/java/org/apache/kafka/raft/RaftProtocol.java

IMHO, I would prefer RaftProtocol as a single file.
I will extract it in the next commit.

frankvicky

Thanks @jsancio @ahuang98

frankvicky · 2025-03-12T07:13:39Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java

@@ -516,15 +521,15 @@ public void testEndQuorumEpochRetriesWhileResigned(boolean withKip853Rpc) throws
    }

    @ParameterizedTest
-    @ValueSource(booleans = { true, false })
-    public void testResignWillCompleteFetchPurgatory(boolean withKip853Rpc) throws Exception {
+    @EnumSource(value = RaftClientTestContext.RaftProtocol.class, names = {"KIP_595_PROTOCOL", "KIP_853_PROTOCOL"})


IMHO, I would prefer RaftProtocol as a single file.
I will extract it in the next commit.

frankvicky · 2025-03-12T07:18:27Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java

+import static org.apache.kafka.raft.RaftClientTestContext.Builder;
+import static org.apache.kafka.raft.RaftClientTestContext.MockListener;
+import static org.apache.kafka.raft.RaftClientTestContext.RaftProtocol;


Ok.
Just for memo: RaftProtocol will be extracted as a single file. I will revert Builder and MockListener

frankvicky · 2025-03-12T07:30:26Z

raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java

@@ -92,25 +97,25 @@ public void testNodeDirectoryId() {
    }

    @ParameterizedTest
-    @ValueSource(booleans = { true, false })
-    public void testInitializeSingleMemberQuorum(boolean withKip853Rpc) throws IOException {
+    @EnumSource(value = RaftProtocol.class, names = {"KIP_595_PROTOCOL", "KIP_853_PROTOCOL"})


I think it could be done in a follow-up PR.
If you agree, I will open a ticket to trace it.

github-actions bot added triage PRs from the community tests Test fixes (including flaky tests) kraft labels Jan 17, 2025

chia7712 added the ci-approved label Jan 17, 2025

frankvicky force-pushed the KAFKA-18561 branch from 2d4e93d to aaaab78 Compare January 18, 2025 01:25

ijuma requested a review from jsancio January 18, 2025 13:39

frankvicky force-pushed the KAFKA-18561 branch 2 times, most recently from c36f9bc to 312c113 Compare January 21, 2025 02:29

jsancio self-assigned this Jan 22, 2025

frankvicky force-pushed the KAFKA-18561 branch from 312c113 to d943da8 Compare January 24, 2025 07:51

github-actions bot added the needs-attention label Jan 25, 2025

frankvicky force-pushed the KAFKA-18561 branch 2 times, most recently from b93875b to fabd9f6 Compare January 26, 2025 02:04

github-actions bot removed the needs-attention label Jan 26, 2025

github-actions bot added the needs-attention label Jan 27, 2025

frankvicky force-pushed the KAFKA-18561 branch 2 times, most recently from 4e56589 to 543467d Compare January 29, 2025 02:55

github-actions bot removed the needs-attention label Jan 29, 2025

github-actions bot added the needs-attention label Jan 30, 2025

frankvicky force-pushed the KAFKA-18561 branch 2 times, most recently from e135aed to 826ba56 Compare January 31, 2025 07:42

ahuang98 suggested changes Jan 31, 2025

View reviewed changes

github-actions bot removed needs-attention triage PRs from the community labels Feb 1, 2025

frankvicky force-pushed the KAFKA-18561 branch 4 times, most recently from cb577db to 5237314 Compare February 12, 2025 04:46

frankvicky force-pushed the KAFKA-18561 branch 2 times, most recently from d990b08 to 8a91dd6 Compare February 17, 2025 09:08

frankvicky force-pushed the KAFKA-18561 branch 2 times, most recently from f4e13da to aa09cd8 Compare February 23, 2025 14:34

KAFKA-18561: Remove withKip853Rpc and replace with withRaftProtocol

d101ea5

JIRA: KAFKA-18561 Remove withKip853Rpc and replace with withRaftProtocol and adjust corresponding test.

frankvicky force-pushed the KAFKA-18561 branch from aa09cd8 to d101ea5 Compare February 25, 2025 11:23

frankvicky added 2 commits March 2, 2025 21:17

Merge branch 'trunk' into KAFKA-18561

c9654ad

Merge branch 'trunk' into KAFKA-18561

2103297

frankvicky added 3 commits March 6, 2025 11:10

Merge branch 'trunk' into KAFKA-18561

b78d546

replace isSupport853 with isReconfiguredSupport

e8eb829

address by comments

6a903f2

ahuang98 reviewed Mar 7, 2025

View reviewed changes

frankvicky added 2 commits March 8, 2025 11:03

Merge branch 'trunk' into KAFKA-18561

495dd18

address by comments

0aca46f

ahuang98 approved these changes Mar 10, 2025

View reviewed changes

jsancio reviewed Mar 10, 2025

View reviewed changes

extract RaftProtocol as a single file and revert some static import

8a7221f

frankvicky commented Mar 12, 2025

View reviewed changes

frankvicky added 3 commits March 12, 2025 15:31

Merge branch 'trunk' into KAFKA-18561

a200b78

apply spotless and enable all raft protocol for KafkaRaftClientTest

6085ab7

Merge branch 'trunk' into KAFKA-18561

f7a73ca

		private static final String KIP_595_PROTOCOL = "KIP_595_PROTOCOL";
		private static final String KIP_853_PROTOCOL = "KIP_853_PROTOCOL";

KAFKA-18561: Remove withKip853Rpc and replace it with withRaftProtocol #18600

Are you sure you want to change the base?

KAFKA-18561: Remove withKip853Rpc and replace it with withRaftProtocol #18600

Conversation

frankvicky commented Jan 17, 2025

Committer Checklist (excluded from commit message)

frankvicky commented Jan 17, 2025

ijuma commented Jan 22, 2025

github-actions bot commented Jan 25, 2025

github-actions bot commented Jan 27, 2025

github-actions bot commented Jan 30, 2025

ahuang98 commented Jan 31, 2025 • edited Loading

ahuang98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuang98 Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuang98 commented Feb 19, 2025

ahuang98 commented Feb 19, 2025

ahuang98 commented Mar 5, 2025

Choose a reason for hiding this comment

ahuang98 Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuang98 left a comment

Choose a reason for hiding this comment

jsancio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankvicky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuang98 commented Jan 31, 2025 •

edited

Loading

ahuang98 Mar 5, 2025 •

edited

Loading

ahuang98 Mar 7, 2025 •

edited

Loading