You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "dajac (via GitHub)" <gi...@apache.org> on 2023/04/11 08:46:04 UTC
[GitHub] [kafka] dajac opened a new pull request, #13536: KAFKA-14462; [6/N] Update Records
dajac opened a new pull request, #13536:
URL: https://github.com/apache/kafka/pull/13536
This patch updates the records introduced by KIP-848.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1162500272
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupMemberMetadataValue.json:
##########
@@ -20,8 +20,6 @@
"validVersions": "0",
"flexibleVersions": "0+",
"fields": [
- { "name": "GroupEpoch", "versions": "0+", "type": "int32",
- "about": "The group epoch." },
Review Comment:
This is not necessary because we have a record for the group epoch.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1162493978
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentValue.json:
##########
@@ -22,16 +22,29 @@
"fields": [
{ "name": "MemberEpoch", "versions": "0+", "type": "int32",
"about": "The member epoch." },
+ { "name": "PreviousMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The previous member epoch." },
Review Comment:
We need this in the case where the epoch bump does not reach the member. In this case, it will retry with the old epoch and we want to accept it.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1162499742
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentValue.json:
##########
@@ -22,16 +22,29 @@
"fields": [
{ "name": "MemberEpoch", "versions": "0+", "type": "int32",
"about": "The member epoch." },
+ { "name": "PreviousMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The previous member epoch." },
+ { "name": "TargetMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The target member epoch." },
+ { "name": "AssignedPartitions", "versions": "0+", "type": "[]TopicPartitions",
+ "about": "The partitions assigned to (owned by) this member." },
+ { "name": "PartitionsPendingRevocation", "versions": "0+", "type": "[]TopicPartitions",
+ "about": "The partitions being revoked by this member." },
+ { "name": "PartitionsPendingAssignment", "versions": "0+", "type": "[]TopicPartitions",
+ "about": "The partitions being assigned to this member." },
Review Comment:
This is the biggest divergence from the KIP. Initially, I wanted to store only the full assignment and to bookkeep those in-memory. I found this extremely confusing during the implementation and error prone as well. It is much better to have three sets instead of ones.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #13536:
URL: https://github.com/apache/kafka/pull/13536
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "hachikuji (via GitHub)" <gi...@apache.org>.
hachikuji commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1164424570
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentValue.json:
##########
@@ -22,16 +22,29 @@
"fields": [
{ "name": "MemberEpoch", "versions": "0+", "type": "int32",
"about": "The member epoch." },
+ { "name": "PreviousMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The previous member epoch." },
Review Comment:
Could we move some of these comments into the "about" documentation?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1164487164
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentValue.json:
##########
@@ -22,16 +22,29 @@
"fields": [
{ "name": "MemberEpoch", "versions": "0+", "type": "int32",
"about": "The member epoch." },
+ { "name": "PreviousMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The previous member epoch." },
Review Comment:
Sure. Updated those when relevant.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1162495603
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentValue.json:
##########
@@ -22,16 +22,29 @@
"fields": [
{ "name": "MemberEpoch", "versions": "0+", "type": "int32",
"about": "The member epoch." },
+ { "name": "PreviousMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The previous member epoch." },
+ { "name": "TargetMemberEpoch", "versions": "0+", "type": "int32",
+ "about": "The target member epoch." },
Review Comment:
We need this in order to know what was the assignment epoch used to compute the assigned partitions, etc. When if diverges with the current assignment epoch, we know that we need to recompute the assignment.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13536: KAFKA-14462; [6/N] Update Records
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13536:
URL: https://github.com/apache/kafka/pull/13536#discussion_r1162500608
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupMemberMetadataValue.json:
##########
@@ -34,6 +32,10 @@
"about": "The list of subscribed topic names." },
{ "name": "SubscribedTopicRegex", "versions": "0+", "nullableVersions": "0+", "type": "string",
"about": "The subscribed topic regular expression." },
+ { "name": "RebalanceTimeoutMs", "type": "int32", "versions": "0+", "default": -1,
+ "about": "The rebalance timeout" },
+ { "name": "ServerAssignor", "versions": "0+", "nullableVersions": "0+", "type": "string",
+ "about": "The server assignor to use; or null if not used." },
Review Comment:
Missing fields.
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupPartitionMetadataValue.json:
##########
@@ -26,6 +26,8 @@
"about": "The list of topic metadata.", "fields": [
{ "name": "TopicId", "versions": "0+", "type": "uuid",
"about": "The topic id." },
+ { "name": "TopicName", "versions": "0+", "type": "string",
+ "about": "The topic name." },
Review Comment:
Missing field.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org