You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "brandboat (via GitHub)" <gi...@apache.org> on 2026/03/01 13:59:27 UTC
[PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
brandboat opened a new pull request, #21607:
URL: https://github.com/apache/kafka/pull/21607
No caller uses find() on ListenerCollection. Removing mapKey=true
changes the generated type to List<Listener>, which lets
BrokerLifecycleManager store an immutable copy via List.copyOf()
instead of the mutable .duplicate(), and avoids wrapping the list
back into a ListenerCollection when building BrokerRegistrationRequestData.
mapKey is a codegen-only annotation with no effect on the wire format,
so RPC compatibility is unaffected.
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2871615375
##########
metadata/src/main/java/org/apache/kafka/metadata/ListenerInfo.java:
##########
@@ -134,7 +134,7 @@ public static ListenerInfo fromControllerRegistrationRecord(
* @return The ListenerInfo object.
*/
public static ListenerInfo fromBrokerRegistrationRequest(
- BrokerRegistrationRequestData.ListenerCollection collection
+ List<BrokerRegistrationRequestData.Listener> collection
) {
LinkedHashMap<String, Endpoint> listeners = new LinkedHashMap<>();
Review Comment:
ugh, I checked on the wrong class. It seems we don't exclude the duplicate items in the serialization layer :(
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2870218307
##########
metadata/src/main/java/org/apache/kafka/metadata/ListenerInfo.java:
##########
@@ -134,7 +134,7 @@ public static ListenerInfo fromControllerRegistrationRecord(
* @return The ListenerInfo object.
*/
public static ListenerInfo fromBrokerRegistrationRequest(
- BrokerRegistrationRequestData.ListenerCollection collection
+ List<BrokerRegistrationRequestData.Listener> collection
) {
LinkedHashMap<String, Endpoint> listeners = new LinkedHashMap<>();
Review Comment:
Thanks for the clarification! I hadn't noticed that before. I'm ok to close out the PR. One thing to point out is that ImplicitLinkedHashMultiCollection seems to only provide reference equality, rather than deep equality for elements.
If we want to enforce unique listener names at the RPC layer, we still need explicit validation, right? Or am I missing something? Is reference equality sufficient here?
https://github.com/apache/kafka/blob/c77a96d13238c3127dc8cf016d437efb5e64bfa9/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java#L32-L35
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21607:
URL: https://github.com/apache/kafka/pull/21607#issuecomment-3985832353
> a test confirming the reading that nothing changes on the wire level?
the `mapKey` does not impact the compatibility. see #21441
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2871274453
##########
metadata/src/main/java/org/apache/kafka/metadata/ListenerInfo.java:
##########
@@ -134,7 +134,7 @@ public static ListenerInfo fromControllerRegistrationRecord(
* @return The ListenerInfo object.
*/
public static ListenerInfo fromBrokerRegistrationRequest(
- BrokerRegistrationRequestData.ListenerCollection collection
+ List<BrokerRegistrationRequestData.Listener> collection
) {
LinkedHashMap<String, Endpoint> listeners = new LinkedHashMap<>();
Review Comment:
Pardon me, could you please explain a bit more on this? ImplicitLinkedHashMultiCollection#addInternal itself doesn't invoke `elementKeysAreEqual`
https://github.com/apache/kafka/blob/11ad60b452edf527723b5d46c63bed76db153ac2/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java#L68-L83
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2871617202
##########
metadata/src/main/java/org/apache/kafka/metadata/ListenerInfo.java:
##########
@@ -134,7 +134,7 @@ public static ListenerInfo fromControllerRegistrationRecord(
* @return The ListenerInfo object.
*/
public static ListenerInfo fromBrokerRegistrationRequest(
- BrokerRegistrationRequestData.ListenerCollection collection
+ List<BrokerRegistrationRequestData.Listener> collection
) {
LinkedHashMap<String, Endpoint> listeners = new LinkedHashMap<>();
Review Comment:
That seems this PR can keep going :)
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat closed pull request #21607: MINOR: remove `mapKey: true` from BrokerRegistrationRequest
URL: https://github.com/apache/kafka/pull/21607
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #21607:
URL: https://github.com/apache/kafka/pull/21607#issuecomment-3988173357
@clolov, @chia7712 What about we adding a new test for MessageGenerator to make sure mapKey doesn't impact the wire level compatibility? My idea is we can generate Java source from two schemas (with/without mapKey: true) and asserts the bytes arrays serialized from both schemas are the same.
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2879657846
##########
clients/src/main/resources/common/message/BrokerRegistrationRequest.json:
##########
@@ -34,7 +34,7 @@
"about": "The incarnation id of the broker process." },
{ "name": "Listeners", "type": "[]Listener",
"about": "The listeners of this broker.", "versions": "0+", "fields": [
- { "name": "Name", "type": "string", "versions": "0+", "mapKey": true,
+ { "name": "Name", "type": "string", "versions": "0+",
Review Comment:
Maybe we should close this PR. My main reason for modifying the protocol was immutability, but I feel consistency is more important now
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2879158452
##########
clients/src/main/resources/common/message/BrokerRegistrationRequest.json:
##########
@@ -34,7 +34,7 @@
"about": "The incarnation id of the broker process." },
{ "name": "Listeners", "type": "[]Listener",
"about": "The listeners of this broker.", "versions": "0+", "fields": [
- { "name": "Name", "type": "string", "versions": "0+", "mapKey": true,
+ { "name": "Name", "type": "string", "versions": "0+",
Review Comment:
Then we have some inconsistencies here, that make me hesitant to move forward. I'm worried mapKey might be some kind of convention in Kafka, despite it seeming redundant in our current xxxCollections, many of them don't even use find/findAll.
Does it make sense to pause here until we understand why it was added in the first place?
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2869681508
##########
metadata/src/main/java/org/apache/kafka/metadata/ListenerInfo.java:
##########
@@ -134,7 +134,7 @@ public static ListenerInfo fromControllerRegistrationRecord(
* @return The ListenerInfo object.
*/
public static ListenerInfo fromBrokerRegistrationRequest(
- BrokerRegistrationRequestData.ListenerCollection collection
+ List<BrokerRegistrationRequestData.Listener> collection
) {
LinkedHashMap<String, Endpoint> listeners = new LinkedHashMap<>();
Review Comment:
It seems I overlooked the usage before. It requires unique names, so `mapKey` actually protects us at the RPC layer. WDYT?
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2876988916
##########
clients/src/main/resources/common/message/BrokerRegistrationRequest.json:
##########
@@ -34,7 +34,7 @@
"about": "The incarnation id of the broker process." },
{ "name": "Listeners", "type": "[]Listener",
"about": "The listeners of this broker.", "versions": "0+", "fields": [
- { "name": "Name", "type": "string", "versions": "0+", "mapKey": true,
+ { "name": "Name", "type": "string", "versions": "0+",
Review Comment:
It seems `Features` does not use the `mapKey` actually. Maybe we could do more refactor in this PR?
`RegisterBrokerRecord` has analogous issue
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #21607:
URL: https://github.com/apache/kafka/pull/21607#issuecomment-3995030535
Close this pr as we don't need it anymore, see https://github.com/apache/kafka/pull/21607#discussion_r2879657846
--
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
Re: [PR] MINOR: remove `mapKey: true` from BrokerRegistrationRequest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21607:
URL: https://github.com/apache/kafka/pull/21607#discussion_r2871254089
##########
metadata/src/main/java/org/apache/kafka/metadata/ListenerInfo.java:
##########
@@ -134,7 +134,7 @@ public static ListenerInfo fromControllerRegistrationRecord(
* @return The ListenerInfo object.
*/
public static ListenerInfo fromBrokerRegistrationRequest(
- BrokerRegistrationRequestData.ListenerCollection collection
+ List<BrokerRegistrationRequestData.Listener> collection
) {
LinkedHashMap<String, Endpoint> listeners = new LinkedHashMap<>();
Review Comment:
```
@Override
public boolean elementKeysAreEqual(Object obj) {
if (!(obj instanceof Listener)) return false;
Listener other = (Listener) obj;
if (this.name == null) {
if (other.name != null) return false;
} else {
if (!this.name.equals(other.name)) return false;
}
return true;
}
```
It compares the `name` field instead of the reference.
--
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