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/02/06 14:31:50 UTC
[GitHub] [kafka] dajac opened a new pull request, #13203: MINOR: Add KIP-848 new `__consumer_offsets` records
dajac opened a new pull request, #13203:
URL: https://github.com/apache/kafka/pull/13203
WIP. https://github.com/apache/kafka/pull/13200 must go first.
### 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] mimaison commented on a diff in pull request #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1098462254
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentKey.json:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupCurrentMemberAssignmentKey",
+ "validVersions": "8",
Review Comment:
Since these are new records, why are we not starting at version 0? This applies to other records too.
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupPartitionMetadataValue.json:
##########
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupPartitionMetadataValue",
+ "validVersions": "0",
+ "flexibleVersions": "0+",
+ "fields": [
+ { "name": "Epoch", "versions": "0+", "type": "int32" },
+ { "name": "TopicPartitionMetadata", "versions": "0+",
+ "type": "[]TopicPartition", "fields": [
Review Comment:
Here `[]TopicPartition` is used for wrapping a `uuid` and an `int32`. [Further down](https://github.com/apache/kafka/pull/13203/files#diff-61c0353c238f00c48dbf08404b1bfa18015faaa64f44e71aa9dc67d66654b17aR25) the same type is used a `uuid` and an `int32[]`. I know these types will mostly be used in generated code but I wonder if trying to use different names would help when people have to go digging in the lowest layers.
Also should we add descriptions to all the fields? Even if it's not part of the public API, and many fields are self explanatory, I think it would help the next person that has to touch this. WDYT?
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentKey.json:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupCurrentMemberAssignmentKey",
+ "validVersions": "8",
+ "flexibleVersions": "none",
Review Comment:
Is there a drawback of making this flexible? There seems to be a mix of flexible and non flexible records.
--
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 #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1098693395
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupPartitionMetadataValue.json:
##########
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupPartitionMetadataValue",
+ "validVersions": "0",
+ "flexibleVersions": "0+",
+ "fields": [
+ { "name": "Epoch", "versions": "0+", "type": "int32" },
+ { "name": "TopicPartitionMetadata", "versions": "0+",
+ "type": "[]TopicPartition", "fields": [
Review Comment:
Let me see what I can do for the naming. Regarding the doc, I agree. I will add them.
--
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 #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1098689954
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentKey.json:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupCurrentMemberAssignmentKey",
+ "validVersions": "8",
Review Comment:
That's a very good question. For the `__consumer_offsets` records, the `version` is used as an unique identifier as well. I know... This is weird. My hope is to change this in the future, possibly before KIP-848 is released, but I will stick to this pattern for now so I can focus on the core implementation first.
--
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 #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #13203:
URL: https://github.com/apache/kafka/pull/13203
--
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 #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1098691309
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentKey.json:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupCurrentMemberAssignmentKey",
+ "validVersions": "8",
+ "flexibleVersions": "none",
Review Comment:
So the records used as keys are not flexible whereas the records used as values are flexible. I suppose that we could make the keys flexible as well but keys should not change so there is little value in doing so.
--
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 #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1099808590
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupMemberMetadataValue.json:
##########
@@ -0,0 +1,53 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupMemberMetadataValue",
+ "validVersions": "0",
+ "flexibleVersions": "0+",
+ "fields": [
+ { "name": "GroupEpoch", "versions": "0+", "type": "int32",
+ "about": "The group epoch." },
+ { "name": "InstanceId", "versions": "0+", "nullableVersions": "0+", "type": "string",
+ "about": "The (optional) instance id." },
+ { "name": "RackId", "versions": "0+", "nullableVersions": "0+", "type": "string",
+ "about": "The (optional) rack id." },
Review Comment:
While adding the doc, I noted that these two fields shout be nullable because they are optional.
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupMemberMetadataValue.json:
##########
@@ -0,0 +1,53 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupMemberMetadataValue",
+ "validVersions": "0",
+ "flexibleVersions": "0+",
+ "fields": [
+ { "name": "GroupEpoch", "versions": "0+", "type": "int32",
+ "about": "The group epoch." },
+ { "name": "InstanceId", "versions": "0+", "nullableVersions": "0+", "type": "string",
+ "about": "The (optional) instance id." },
+ { "name": "RackId", "versions": "0+", "nullableVersions": "0+", "type": "string",
+ "about": "The (optional) rack id." },
Review Comment:
While adding the doc, I noted that these two fields shout be nullable because they are optional. I will update the KIP.
--
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 pull request #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #13203:
URL: https://github.com/apache/kafka/pull/13203#issuecomment-1420381203
Hey @mimaison. As you reviewed https://github.com/apache/kafka/pull/13200, do you mind reviewing this one?
--
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 #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1099807950
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupPartitionMetadataValue.json:
##########
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupPartitionMetadataValue",
+ "validVersions": "0",
+ "flexibleVersions": "0+",
+ "fields": [
+ { "name": "Epoch", "versions": "0+", "type": "int32" },
+ { "name": "TopicPartitionMetadata", "versions": "0+",
+ "type": "[]TopicPartition", "fields": [
Review Comment:
Renamed that field and added doc to add fields.
--
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] mimaison commented on a diff in pull request #13203: KAFKA-14048: Add new `__consumer_offsets` records from KIP-848
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #13203:
URL: https://github.com/apache/kafka/pull/13203#discussion_r1100494370
##########
group-coordinator/src/main/resources/common/message/ConsumerGroupCurrentMemberAssignmentKey.json:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// KIP-848 is in development. This schema is subject to non-backwards-compatible changes.
+{
+ "type": "data",
+ "name": "ConsumerGroupCurrentMemberAssignmentKey",
+ "validVersions": "8",
Review Comment:
I see, thanks for the explaination
--
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