[GitHub] [kafka] jolshan commented on a diff in pull request #13323: KAFKA-14617: Add ReplicaState to FetchRequest

2023-03-14 Thread via GitHub 


jolshan commented on code in PR #13323:
URL: https://github.com/apache/kafka/pull/13323#discussion_r1136245326


##
clients/src/main/resources/common/message/FetchRequest.json:
##
@@ -50,14 +50,23 @@
   // Version 13 replaces topic names with topic IDs (KIP-516). May return 
UNKNOWN_TOPIC_ID error code.
   //
   // Version 14 is the same as version 13 but it also receives a new error 
called OffsetMovedToTieredStorageException(KIP-405)
-  "validVersions": "0-14",
+  //
+  // 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",
   "flexibleVersions": "12+",
   "fields": [
 { "name": "ClusterId", "type": "string", "versions": "12+", 
"nullableVersions": "12+", "default": "null",
   "taggedVersions": "12+", "tag": 0, "ignorable": true,
   "about": "The clusterId if known. This is used to validate metadata 
fetches prior to broker registration." },
-{ "name": "ReplicaId", "type": "int32", "versions": "0+", "entityType": 
"brokerId",
+{ "name": "ReplicaId", "type": "int32", "versions": "0-14", "default": 
"-1", "entityType": "brokerId",
   "about": "The broker ID of the follower, of -1 if this request is from a 
consumer." },
+{ "name": "ReplicaState", "type": "ReplicaState", "taggedVersions":"15+", 
"tag": 1, "fields": [

Review Comment:
   > Yes, when constructing an instance, it will do this.replicaState = new 
ReplicaState()
   
   The builder is still going to write to these feels as far as I can see in 
the code. (FetchRequest line 266) Its just the case that these happen to be the 
defaults and so the state is equal, right?



-- 
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

[GitHub] [kafka] jolshan commented on a diff in pull request #13323: KAFKA-14617: Add ReplicaState to FetchRequest

2023-03-14 Thread via GitHub 


jolshan commented on code in PR #13323:
URL: https://github.com/apache/kafka/pull/13323#discussion_r1136221498


##
clients/src/main/resources/common/message/FetchRequest.json:
##
@@ -50,14 +50,23 @@
   // Version 13 replaces topic names with topic IDs (KIP-516). May return 
UNKNOWN_TOPIC_ID error code.
   //
   // Version 14 is the same as version 13 but it also receives a new error 
called OffsetMovedToTieredStorageException(KIP-405)
-  "validVersions": "0-14",
+  //
+  // 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",
   "flexibleVersions": "12+",
   "fields": [
 { "name": "ClusterId", "type": "string", "versions": "12+", 
"nullableVersions": "12+", "default": "null",
   "taggedVersions": "12+", "tag": 0, "ignorable": true,
   "about": "The clusterId if known. This is used to validate metadata 
fetches prior to broker registration." },
-{ "name": "ReplicaId", "type": "int32", "versions": "0+", "entityType": 
"brokerId",
+{ "name": "ReplicaId", "type": "int32", "versions": "0-14", "default": 
"-1", "entityType": "brokerId",
   "about": "The broker ID of the follower, of -1 if this request is from a 
consumer." },
+{ "name": "ReplicaState", "type": "ReplicaState", "taggedVersions":"15+", 
"tag": 1, "fields": [

Review Comment:
   Also, does the builder still populate the field? Does it automatically 
remove it if we keep the defaults?



-- 
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

[GitHub] [kafka] jolshan commented on a diff in pull request #13323: KAFKA-14617: Add ReplicaState to FetchRequest

2023-03-14 Thread via GitHub 


jolshan commented on code in PR #13323:
URL: https://github.com/apache/kafka/pull/13323#discussion_r1136220531


##
clients/src/main/resources/common/message/FetchRequest.json:
##
@@ -50,14 +50,23 @@
   // Version 13 replaces topic names with topic IDs (KIP-516). May return 
UNKNOWN_TOPIC_ID error code.
   //
   // Version 14 is the same as version 13 but it also receives a new error 
called OffsetMovedToTieredStorageException(KIP-405)
-  "validVersions": "0-14",
+  //
+  // 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",
   "flexibleVersions": "12+",
   "fields": [
 { "name": "ClusterId", "type": "string", "versions": "12+", 
"nullableVersions": "12+", "default": "null",
   "taggedVersions": "12+", "tag": 0, "ignorable": true,
   "about": "The clusterId if known. This is used to validate metadata 
fetches prior to broker registration." },
-{ "name": "ReplicaId", "type": "int32", "versions": "0+", "entityType": 
"brokerId",
+{ "name": "ReplicaId", "type": "int32", "versions": "0-14", "default": 
"-1", "entityType": "brokerId",
   "about": "The broker ID of the follower, of -1 if this request is from a 
consumer." },
+{ "name": "ReplicaState", "type": "ReplicaState", "taggedVersions":"15+", 
"tag": 1, "fields": [

Review Comment:
   I see. So we still rely on IBP to know the version?



-- 
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

[GitHub] [kafka] jolshan commented on a diff in pull request #13323: KAFKA-14617: Add ReplicaState to FetchRequest

2023-03-14 Thread via GitHub 


jolshan commented on code in PR #13323:
URL: https://github.com/apache/kafka/pull/13323#discussion_r1136148988


##
clients/src/main/resources/common/message/FetchRequest.json:
##
@@ -50,14 +50,23 @@
   // Version 13 replaces topic names with topic IDs (KIP-516). May return 
UNKNOWN_TOPIC_ID error code.
   //
   // Version 14 is the same as version 13 but it also receives a new error 
called OffsetMovedToTieredStorageException(KIP-405)
-  "validVersions": "0-14",
+  //
+  // 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",
   "flexibleVersions": "12+",
   "fields": [
 { "name": "ClusterId", "type": "string", "versions": "12+", 
"nullableVersions": "12+", "default": "null",
   "taggedVersions": "12+", "tag": 0, "ignorable": true,
   "about": "The clusterId if known. This is used to validate metadata 
fetches prior to broker registration." },
-{ "name": "ReplicaId", "type": "int32", "versions": "0+", "entityType": 
"brokerId",
+{ "name": "ReplicaId", "type": "int32", "versions": "0-14", "default": 
"-1", "entityType": "brokerId",
   "about": "The broker ID of the follower, of -1 if this request is from a 
consumer." },
+{ "name": "ReplicaState", "type": "ReplicaState", "taggedVersions":"15+", 
"tag": 1, "fields": [

Review Comment:
   Sorry if I missed this conversation, but why is this tagged? Is it to avoid 
the IBP bump? We would still need it if we no longer set replica ID right?



-- 
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

[GitHub] [kafka] jolshan commented on a diff in pull request #13323: KAFKA-14617: Add ReplicaState to FetchRequest

2023-03-14 Thread via GitHub 


jolshan commented on code in PR #13323:
URL: https://github.com/apache/kafka/pull/13323#discussion_r1136146308


##
clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java:
##
@@ -302,6 +335,10 @@ public String toString() {
 }
 }
 
+public static int replicaId(FetchRequestData fetchRequestData) {
+return fetchRequestData.replicaId() != -1 ? 
fetchRequestData.replicaId() : fetchRequestData.replicaState().replicaId();

Review Comment:
   In the case of a consumer, we always read replica state (the second part of 
the statement).
   I don't think there are any bugs here, but it might be good to keep in mind.



-- 
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

[GitHub] [kafka] jolshan commented on a diff in pull request #13323: KAFKA-14617; Add ReplicaState to FetchRequest.

2023-03-06 Thread via GitHub 


jolshan commented on code in PR #13323:
URL: https://github.com/apache/kafka/pull/13323#discussion_r1126944890


##
clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java:
##
@@ -337,8 +362,27 @@ public AbstractResponse getErrorResponse(int 
throttleTimeMs, Throwable e) {
 .setResponses(topicResponseList));
 }
 
+public String clusterId() {
+return data.clusterId();
+}
+
+public List topics() {
+return data.topics();
+}
+
+public int maxWaitMs() {
+return data.maxWaitMs();
+}

Review Comment:
   The data object is public, so we may just be able to use that.



-- 
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