KAFKA-14617: Add ReplicaState to FetchRequest #13323

CalvinConfluent · 2023-03-02T02:55:24Z

As the first part of the KIP-903, it updates the FetchRequest:

Deprecate the ReplicaId field
Create a new tagged field ReplicaState with ReplicaId and ReplicaEpoch
Bump the FetchRequest version to 15
Bump metadata version to 3.5-IV1

https://issues.apache.org/jira/browse/KAFKA-14617

dajac

@CalvinConfluent Thanks for the PR. I made a quick pass on it and I left a few comments and questions.

raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java

raft/src/main/java/org/apache/kafka/raft/RaftRequest.java

storage/src/main/java/org/apache/kafka/storage/internals/log/FetchParams.java

raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java

clients/src/main/resources/common/message/FetchRequest.json

clients/src/main/resources/common/message/FetchResponse.json

server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java

CalvinConfluent · 2023-03-02T17:36:18Z

@dajac I think the major question is how the KafkaRaftClient can correctly consume the FetchRequest.
The current PR makes quite some efforts to get the FetchRequest apiVersion and then parse the FetchRequest.
If this way is too much trouble, I can think of another one:

Change the FetchRequest old ReplicaId default value to -1.
Then when the KafkaRaftClient receives the request, it can tell the ReplicaId by comparing the value of the old ReplicaId field and the new one(it has default value -1).

dajac · 2023-03-02T17:46:44Z

@CalvinConfluent Thanks for the explanation. I agree that we have two options on the table: 1) pass the api version or even the header object; or 2) rely on the default sentinel value to read the correct field. Personally, I lean towards 2) here because it is simpler. It would be great to run this by @hachikuji as the authored this part.

CalvinConfluent · 2023-03-07T23:32:20Z

Changing the old ReplicaId field default value to -1. In this way, we can easily extract the replicaId from the FetchRequest without knowing the API version. Simple and minimal work.

dajac

@CalvinConfluent Thanks for the update. I left a few more comments.

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java

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

core/src/test/scala/unit/kafka/server/KafkaApisTest.scala

core/src/test/scala/unit/kafka/server/ReplicaFetcherThreadTest.scala

core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala

dajac · 2023-03-09T17:21:25Z

@CalvinConfluent Could you rebase the PR? There are a few conflicts.

CalvinConfluent · 2023-03-09T17:45:06Z

@dajac Done.

dajac

@CalvinConfluent Thanks for the update. I left a few more comments.

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java

clients/src/test/java/org/apache/kafka/common/requests/FetchRequestTest.java

core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala

core/src/test/scala/kafka/server/RemoteLeaderEndPointTest.scala

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala

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

raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java

dajac

@CalvinConfluent Thanks for the update. I left a few more comments.

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java

core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala

core/src/test/scala/kafka/server/RemoteLeaderEndPointTest.scala

raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java

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

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java

clients/src/main/resources/common/message/FetchRequest.json

dajac

@CalvinConfluent Thanks for the update and your patience on this one. I left a few more nits. We should be good to go when they are addressed.

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java

clients/src/test/java/org/apache/kafka/common/requests/FetchRequestTest.java

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

dajac · 2023-03-15T19:33:40Z

@CalvinConfluent Could you rebase as well?

dajac

LGTM, thanks!

dajac · 2023-03-16T13:01:27Z

Failed tests are not related to this PR:

[Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_17_and_Scala_2_13____1__Type_ZK__Name_testDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
[Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_17_and_Scala_2_13____1__Type_ZK__Name_testDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT_2/)
[Build / JDK 11 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_11_and_Scala_2_13____1__Type_ZK__Name_testDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
[Build / JDK 11 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_11_and_Scala_2_13____1__Type_ZK__Name_testDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT_2/)
[Build / JDK 8 and Scala 2.12 / kafka.admin.LeaderElectionCommandTest.[3] Type=ZK, Name=testPathToJsonFile, MetadataVersion=3.5-IV1, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.admin/LeaderElectionCommandTest/Build___JDK_8_and_Scala_2_12____3__Type_ZK__Name_testPathToJsonFile__MetadataVersion_3_5_IV1__Security_PLAINTEXT/)
[Build / JDK 8 and Scala 2.12 / kafka.admin.TopicCommandIntegrationTest.testTopicDeletion(String).quorum=kraft](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.admin/TopicCommandIntegrationTest/Build___JDK_8_and_Scala_2_12___testTopicDeletion_String__quorum_kraft/)
[Build / JDK 8 and Scala 2.12 / kafka.server.DynamicBrokerReconfigurationTest.testTrustStoreAlter(String).quorum=kraft](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.server/DynamicBrokerReconfigurationTest/Build___JDK_8_and_Scala_2_12___testTrustStoreAlter_String__quorum_kraft/)
[Build / JDK 8 and Scala 2.12 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_8_and_Scala_2_12____1__Type_ZK__Name_testDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
[Build / JDK 8 and Scala 2.12 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/view/change-requests/job/PR-13323/16/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_8_and_Scala_2_12____1__Type_ZK__Name_testDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT_2/)

I am going to merge it to trunk.

dajac changed the title ~~Add ReplicaState to FetchRequest.~~ KAFKA-14617; Add ReplicaState to FetchRequest. Mar 2, 2023

dajac self-requested a review March 2, 2023 10:04

dajac reviewed Mar 2, 2023

View reviewed changes

CalvinConfluent force-pushed the calvin.kafka-14617-1 branch from a304d91 to 6b98e88 Compare March 7, 2023 23:01

CalvinConfluent changed the title ~~KAFKA-14617; Add ReplicaState to FetchRequest.~~ KAFKA-14617 Add ReplicaState to FetchRequest. Mar 7, 2023

CalvinConfluent force-pushed the calvin.kafka-14617-1 branch 2 times, most recently from 8283907 to ad2f187 Compare March 7, 2023 23:30

dajac reviewed Mar 8, 2023

View reviewed changes

CalvinConfluent requested a review from dajac March 9, 2023 02:56

CalvinConfluent force-pushed the calvin.kafka-14617-1 branch from 4de4992 to 81c6ba6 Compare March 9, 2023 17:43

dajac reviewed Mar 10, 2023

View reviewed changes

dajac changed the title ~~KAFKA-14617 Add ReplicaState to FetchRequest.~~ KAFKA-14617: Add ReplicaState to FetchRequest. Mar 10, 2023

dajac changed the title ~~KAFKA-14617: Add ReplicaState to FetchRequest.~~ KAFKA-14617: Add ReplicaState to FetchRequest Mar 10, 2023

Hangleton reviewed Mar 10, 2023

View reviewed changes

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java Outdated Show resolved Hide resolved

CalvinConfluent requested review from Hangleton, dajac and jolshan and removed request for Hangleton, dajac and jolshan March 11, 2023 04:04

dajac reviewed Mar 14, 2023

View reviewed changes

CalvinConfluent requested a review from dajac March 14, 2023 18:48

jolshan reviewed Mar 14, 2023

View reviewed changes

clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java Show resolved Hide resolved

jolshan reviewed Mar 14, 2023

View reviewed changes

clients/src/main/resources/common/message/FetchRequest.json Show resolved Hide resolved

dajac reviewed Mar 15, 2023

View reviewed changes

CalvinConfluent added 9 commits March 15, 2023 13:03

Add ReplicaState to Fetch Request

4213276

Update old ReplicaId default value to -1

d35fbfc

Address comments

66efd44

Minor

04d4183

Address comments

3e0b284

Minor

49693d1

More comments

d76e1d7

Comments

b58916e

Minor

ec4689e

CalvinConfluent force-pushed the calvin.kafka-14617-1 branch from 3fe2867 to ec4689e Compare March 15, 2023 20:07

CalvinConfluent requested a review from dajac March 15, 2023 20:13

dajac approved these changes Mar 16, 2023

View reviewed changes

dajac merged commit 79b5f7f into apache:trunk Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14617: Add ReplicaState to FetchRequest #13323

KAFKA-14617: Add ReplicaState to FetchRequest #13323

CalvinConfluent commented Mar 2, 2023

dajac left a comment

CalvinConfluent commented Mar 2, 2023 •

edited

dajac commented Mar 2, 2023

CalvinConfluent commented Mar 7, 2023

dajac left a comment

dajac commented Mar 9, 2023

CalvinConfluent commented Mar 9, 2023

dajac left a comment

dajac left a comment

dajac left a comment

dajac commented Mar 15, 2023

dajac left a comment

dajac commented Mar 16, 2023

KAFKA-14617: Add ReplicaState to FetchRequest #13323

KAFKA-14617: Add ReplicaState to FetchRequest #13323

Conversation

CalvinConfluent commented Mar 2, 2023

dajac left a comment

Choose a reason for hiding this comment

CalvinConfluent commented Mar 2, 2023 • edited

dajac commented Mar 2, 2023

CalvinConfluent commented Mar 7, 2023

dajac left a comment

Choose a reason for hiding this comment

dajac commented Mar 9, 2023

CalvinConfluent commented Mar 9, 2023

dajac left a comment

Choose a reason for hiding this comment

dajac left a comment

Choose a reason for hiding this comment

dajac left a comment

Choose a reason for hiding this comment

dajac commented Mar 15, 2023

dajac left a comment

Choose a reason for hiding this comment

dajac commented Mar 16, 2023

CalvinConfluent commented Mar 2, 2023 •

edited