KAFKA-15661: KIP-951: protocol changes #14627

chb2ab · 2023-10-24T21:19:32Z

Separating out the protocol changes from #14444 in an effort to more quickly unblock the client side PR.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client

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

Testing

./gradlew core:test --tests kafka.server.KafkaApisTest

msn-tldr · 2023-10-25T09:28:52Z

@chb2ab lgtm, thanks for raising this!

jolshan · 2023-10-25T22:40:42Z

Hey there. If this is KAFKA-15661, then the title of the PR should be
KAFKA-15661: KIP-951: Server side and protocol changes

jolshan · 2023-10-25T22:41:28Z

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

@@ -45,7 +45,9 @@
  // Version 14 is the same as version 13 but it also receives a new error called OffsetMovedToTieredStorageException (KIP-405)
  //
  // Version 15 is the same as version 14 (KIP-903).
-  "validVersions": "0-15",
+  //
+  // Version 16 adds the 'NodeEndpoints' field.


please include the kip as was done for the other versions

jolshan · 2023-10-25T22:41:54Z

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

@@ -102,6 +104,15 @@
          "about": "The preferred read replica for the consumer to use on its next fetch request"},
        { "name": "Records", "type": "records", "versions": "0+", "nullableVersions": "0+", "about": "The record data."}
      ]}
+    ]},
+    { "name": "NodeEndpoints", "type": "[]NodeEndpoint", "versions": "16+", "taggedVersions": "16+", "tag": 0,


let's not reuse the tags as I mentioned in the other PR.

I replied to this in the other PR #14444 (comment)

it looks like tags are scoped to the list level so this isn't really the same tag. They also need to be contiguous within their scope so this gives an error if I try to tag NodeEndpoints to something other than 0

Sharing the comment here for clarity.

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

jolshan · 2023-10-25T22:45:22Z

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

@@ -360,7 +360,9 @@ public short partitionRecordVersion() {
    }

    public short fetchRequestVersion() {
-        if (this.isAtLeast(IBP_3_5_IV1)) {
+        if (this.isAtLeast(IBP_3_7_IV0)) {


Does this work because all the new fields are tagged (so no real changes in handling)?

This is supposed to be the first approach mentioned in this comment #14444 (comment)

My understanding is clusters would first be upgraded to 3.7 and then the IBP would be bumped after all the brokers are upgraded, but please correct me if that's wrong. You're right though, all the fields are tagged and there's no change in handling on the broker side.

I was referring to whether the code here that we want to merge will break the build if we haven't implemented the new fields.

Oh, yeah we shouldn't need any other changes, the fields are all tagged and not getting used anywhere so we can leave them as their default values.

I just realized that if we set the version as unstable, we may not be able to use it here. 🤦‍♀️ Maybe we should remove the unstable version true if this causes issues in tests.

Sorry for confusion.

msn-tldr · 2023-10-26T11:55:34Z

@chb2ab CMIIW, but @jolshan i think scope of the PR is just protocol changes, not server-side changes. Intention is to merge the protocol-changes soon, so client-side changes can also be reviewed utilising the change.
I see 2 options to make this happen

Merge PR KAFKA-15661: KIP-951: Server side changes #14444, if its ready.
If 1. isn't feasible, then only merge the protocol changes with the current PR.

Going through PR 14444, it seems close, @jolshan what do you think? Is it possible to merge in next 1-2 days?

chb2ab · 2023-10-26T14:31:20Z

I updated the title, this would be a partial implementation of KAFKA-15661.

I replied to the other comments as well and incorporated them into #14444, if there are no more comments there it would be easier to just merge that.

jolshan · 2023-10-26T17:49:00Z

I still need to go over the changes in #14444. This would still probably be faster.

jolshan · 2023-10-27T17:57:49Z

@chb2ab sorry for the confusion. I realized that marking the version unstable will probably cause issues if the ibp suggests using that version. Since the tagged fields are the only difference, let's remove the unstable version flag. Sorry for the back and forth. After that, I will check the tests again and hopefully this will be good to go.

#14627 (comment)

chb2ab · 2023-10-27T19:24:11Z

@jolshan np, I reverted that change.

jolshan · 2023-10-27T20:17:14Z

I will just watch the build now. Thanks @chb2ab

This reverts commit d53c43f589bf66cebe4c1fbee92687d5f2e77a6a.

This is the server side changes to populate the fields in KIP-951. On NOT_LEADER_OR_FOLLOWER errors in both FETCH and PRODUCE the new leader ID and epoch are retrieved from the local cache through ReplicaManager and included in the response, falling back to the metadata cache if they are unavailable there. The endpoint for the new leader is retrieved from the metadata cache. The new fields are all optional (tagged) and an IBP bump was required. https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client https://issues.apache.org/jira/browse/KAFKA-15661 Protocol changes: #14627 Testing Benchmarking described here https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client#KIP951:Leaderdiscoveryoptimisationsfortheclient-BenchmarkResults ./gradlew core:test --tests kafka.server.KafkaApisTest Reviewers: Justine Olshan <jolshan@confluent.io>, David Jacot <djacot@confluent.io>, Jason Gustafson <jason@confluent.io>, Fred Zheng <zhengyd2014@gmail.com>, Mayank Shekhar Narula <mayanks.narula@gmail.com>, Yang Yang <yayang@uber.com>, David Mao <dmao@confluent.io>, Kirk True <ktrue@confluent.io>

This is the server side changes to populate the fields in KIP-951. On NOT_LEADER_OR_FOLLOWER errors in both FETCH and PRODUCE the new leader ID and epoch are retrieved from the local cache through ReplicaManager and included in the response, falling back to the metadata cache if they are unavailable there. The endpoint for the new leader is retrieved from the metadata cache. The new fields are all optional (tagged) and an IBP bump was required. https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client https://issues.apache.org/jira/browse/KAFKA-15661 Protocol changes: apache#14627 Testing Benchmarking described here https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client#KIP951:Leaderdiscoveryoptimisationsfortheclient-BenchmarkResults ./gradlew core:test --tests kafka.server.KafkaApisTest Reviewers: Justine Olshan <jolshan@confluent.io>, David Jacot <djacot@confluent.io>, Jason Gustafson <jason@confluent.io>, Fred Zheng <zhengyd2014@gmail.com>, Mayank Shekhar Narula <mayanks.narula@gmail.com>, Yang Yang <yayang@uber.com>, David Mao <dmao@confluent.io>, Kirk True <ktrue@confluent.io>

Separating out the protocol changes from apache#14444 in an effort to more quickly unblock the client side PR. This is the protocol changes to populate the fields in KIP-951. On NOT_LEADER_OR_FOLLOWER errors in both FETCH and PRODUCE the new leader ID and epoch are included in the response. The endpoint for the new leader is retrieved from the metadata cache. The new fields are all optional (tagged) and an IBP bump is required. https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client Reviewers: Justine Olshan <jolshan@confluent.io>, Mayank Shekhar Narula <mayanks.narula@gmail.com>

This is the server side changes to populate the fields in KIP-951. On NOT_LEADER_OR_FOLLOWER errors in both FETCH and PRODUCE the new leader ID and epoch are retrieved from the local cache through ReplicaManager and included in the response, falling back to the metadata cache if they are unavailable there. The endpoint for the new leader is retrieved from the metadata cache. The new fields are all optional (tagged) and an IBP bump was required. https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client https://issues.apache.org/jira/browse/KAFKA-15661 Protocol changes: apache#14627 Testing Benchmarking described here https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client#KIP951:Leaderdiscoveryoptimisationsfortheclient-BenchmarkResults ./gradlew core:test --tests kafka.server.KafkaApisTest Reviewers: Justine Olshan <jolshan@confluent.io>, David Jacot <djacot@confluent.io>, Jason Gustafson <jason@confluent.io>, Fred Zheng <zhengyd2014@gmail.com>, Mayank Shekhar Narula <mayanks.narula@gmail.com>, Yang Yang <yayang@uber.com>, David Mao <dmao@confluent.io>, Kirk True <ktrue@confluent.io>

Separating out the protocol changes from apache#14444 in an effort to more quickly unblock the client side PR. This is the protocol changes to populate the fields in KIP-951. On NOT_LEADER_OR_FOLLOWER errors in both FETCH and PRODUCE the new leader ID and epoch are included in the response. The endpoint for the new leader is retrieved from the metadata cache. The new fields are all optional (tagged) and an IBP bump is required. https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client Reviewers: Justine Olshan <jolshan@confluent.io>, Mayank Shekhar Narula <mayanks.narula@gmail.com>

This is the server side changes to populate the fields in KIP-951. On NOT_LEADER_OR_FOLLOWER errors in both FETCH and PRODUCE the new leader ID and epoch are retrieved from the local cache through ReplicaManager and included in the response, falling back to the metadata cache if they are unavailable there. The endpoint for the new leader is retrieved from the metadata cache. The new fields are all optional (tagged) and an IBP bump was required. https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client https://issues.apache.org/jira/browse/KAFKA-15661 Protocol changes: apache#14627 Testing Benchmarking described here https://cwiki.apache.org/confluence/display/KAFKA/KIP-951%3A+Leader+discovery+optimisations+for+the+client#KIP951:Leaderdiscoveryoptimisationsfortheclient-BenchmarkResults ./gradlew core:test --tests kafka.server.KafkaApisTest Reviewers: Justine Olshan <jolshan@confluent.io>, David Jacot <djacot@confluent.io>, Jason Gustafson <jason@confluent.io>, Fred Zheng <zhengyd2014@gmail.com>, Mayank Shekhar Narula <mayanks.narula@gmail.com>, Yang Yang <yayang@uber.com>, David Mao <dmao@confluent.io>, Kirk True <ktrue@confluent.io>

hudeqi added producer consumer labels Oct 25, 2023

msn-tldr mentioned this pull request Oct 25, 2023

KAFKA-15627: KIP-951's Leader discovery optimisations on the client #14564

Closed

3 tasks

jolshan reviewed Oct 25, 2023

View reviewed changes

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

jolshan reviewed Oct 25, 2023

View reviewed changes

chb2ab changed the title ~~KIP-951: protocol changes~~ KAFKA-15661: KIP-951: protocol changes Oct 26, 2023

chb2ab added 5 commits October 30, 2023 14:31

KIP-951: protocol changes

71e8692

bump fetch version on MV

d46fe38

updating comments with kip

a983ea3

marking unstable

6be95ce

Revert "marking unstable"

23e3026

This reverts commit d53c43f589bf66cebe4c1fbee92687d5f2e77a6a.

chb2ab force-pushed the chb2ab-kip951-serverside-5 branch from 2d36fb6 to 23e3026 Compare October 30, 2023 18:41

jolshan merged commit c8f687a into apache:trunk Nov 1, 2023
1 check failed

chb2ab mentioned this pull request Nov 1, 2023

KAFKA-15661: KIP-951: Server side changes #14444

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-15661: KIP-951: protocol changes #14627

KAFKA-15661: KIP-951: protocol changes #14627

chb2ab commented Oct 24, 2023

msn-tldr commented Oct 25, 2023

jolshan commented Oct 25, 2023

jolshan Oct 25, 2023

chb2ab Oct 26, 2023

jolshan Oct 25, 2023

chb2ab Oct 26, 2023

jolshan Oct 26, 2023

jolshan Oct 25, 2023

chb2ab Oct 26, 2023

jolshan Oct 26, 2023

chb2ab Oct 26, 2023

jolshan Oct 26, 2023

msn-tldr commented Oct 26, 2023

chb2ab commented Oct 26, 2023

jolshan commented Oct 26, 2023

jolshan commented Oct 27, 2023

chb2ab commented Oct 27, 2023

jolshan commented Oct 27, 2023

KAFKA-15661: KIP-951: protocol changes #14627

KAFKA-15661: KIP-951: protocol changes #14627

Conversation

chb2ab commented Oct 24, 2023

Testing

msn-tldr commented Oct 25, 2023

jolshan commented Oct 25, 2023

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

Choose a reason for hiding this comment

msn-tldr commented Oct 26, 2023

chb2ab commented Oct 26, 2023

jolshan commented Oct 26, 2023

jolshan commented Oct 27, 2023

chb2ab commented Oct 27, 2023

jolshan commented Oct 27, 2023