Re: [PR] KIP-951: protocol changes [kafka]
chb2ab commented on code in PR #14627: URL: https://github.com/apache/kafka/pull/14627#discussion_r1373265206 ## clients/src/main/resources/common/message/FetchRequest.json: ## @@ -53,7 +53,9 @@ // // Version 15 adds the ReplicaState which includes new field ReplicaEpoch and the ReplicaId. Also, // deprecate the old ReplicaId field and set its default value to -1. (KIP-903) - "validVersions": "0-15", + // + // Version 16 is the same as version 15. + "validVersions": "0-16", Review Comment: I'm not sure about this. The intention is for all the new fields to be optional, the client should be able to handle them not being populated (by doing a full metadata refresh), so it doesn't seem unstable to me. For reference FetchResponse already has the CurrentLeader field we're adding to ProduceResponse, we just never used it for anything. -- 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] KIP-951: protocol changes [kafka]
chb2ab commented on code in PR #14627: URL: https://github.com/apache/kafka/pull/14627#discussion_r1373258152 ## 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. Review Comment: done -- 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] KIP-951: protocol changes [kafka]
chb2ab commented on code in PR #14627:
URL: https://github.com/apache/kafka/pull/14627#discussion_r1373245183
##
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,
Review Comment:
I replied to this in the other PR
https://github.com/apache/kafka/pull/1#discussion_r1367116597
--
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] KIP-951: protocol changes [kafka]
chb2ab commented on code in PR #14627:
URL: https://github.com/apache/kafka/pull/14627#discussion_r1373238109
##
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)) {
Review Comment:
This is supposed to be the first approach mentioned in this comment
https://github.com/apache/kafka/pull/1#discussion_r1360994577
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.
--
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] KIP-951: protocol changes [kafka]
msn-tldr commented on PR #14627: URL: https://github.com/apache/kafka/pull/14627#issuecomment-1780966602 @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 1. Merge PR https://github.com/apache/kafka/pull/1, if its ready. 2. If 1. isn't feasible, then only merge the protocol changes with the current PR. Going through PR 1, it seems close, @jolshan what do you think? Is it possible to merge in next 1-2 days? -- 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] KIP-951: protocol changes [kafka]
jolshan commented on code in PR #14627:
URL: https://github.com/apache/kafka/pull/14627#discussion_r1372394072
##
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)) {
Review Comment:
Does this work because all the new fields are tagged (so no real changes in
handling)?
--
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] KIP-951: protocol changes [kafka]
jolshan commented on code in PR #14627: URL: https://github.com/apache/kafka/pull/14627#discussion_r1372393724 ## clients/src/main/resources/common/message/FetchRequest.json: ## @@ -53,7 +53,9 @@ // // Version 15 adds the ReplicaState which includes new field ReplicaEpoch and the ReplicaId. Also, // deprecate the old ReplicaId field and set its default value to -1. (KIP-903) - "validVersions": "0-15", + // + // Version 16 is the same as version 15. + "validVersions": "0-16", Review Comment: we should mark these APIs as in development by specifying `"latestVersionUnstable":true` See https://github.com/apache/kafka/pull/14046 for an example. -- 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] KIP-951: protocol changes [kafka]
jolshan commented on code in PR #14627:
URL: https://github.com/apache/kafka/pull/14627#discussion_r1372392217
##
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,
Review Comment:
let's not reuse the tags as I mentioned in the other PR.
--
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] KIP-951: protocol changes [kafka]
jolshan commented on code in PR #14627: URL: https://github.com/apache/kafka/pull/14627#discussion_r1372391952 ## 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. Review Comment: please include the kip as was done for the other versions -- 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] KIP-951: protocol changes [kafka]
jolshan commented on PR #14627: URL: https://github.com/apache/kafka/pull/14627#issuecomment-1780155389 Hey there. If this is KAFKA-15661, then the title of the PR should be `KAFKA-15661: KIP-951: Server side and protocol changes` -- 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] KIP-951: protocol changes [kafka]
msn-tldr commented on PR #14627: URL: https://github.com/apache/kafka/pull/14627#issuecomment-1778870153 @chb2ab lgtm, thanks for raising this! -- 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] KIP-951: protocol changes [kafka]
chb2ab opened a new pull request, #14627: URL: https://github.com/apache/kafka/pull/14627 Separating out the protocol changes from https://github.com/apache/kafka/pull/1 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` -- 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
