cmccabe commented on code in PR #16183: URL: https://github.com/apache/kafka/pull/16183#discussion_r1684705617
########## core/src/main/scala/kafka/server/BrokerLifecycleManager.scala: ########## @@ -381,10 +381,12 @@ class BrokerLifecycleManager( private def sendBrokerRegistration(): Unit = { val features = new BrokerRegistrationRequestData.FeatureCollection() _supportedFeatures.asScala.foreach { - case (name, range) => features.add(new BrokerRegistrationRequestData.Feature(). + // Do not include features with the range 0-0. + case (name, range) if range.max() > 0 => features.add(new BrokerRegistrationRequestData.Feature(). Review Comment: From my point of view: 1. We should never report a `0-0` supported versions range in an RPC. There is no point! This applies to both `ApiVersionsResponse` and `BrokerRegistrationRequest`. 2. Obviously a `1-0` range is even worse, being incorrect as well as useless. 3. Having a HashMap or something that contains a `0-0` range is fine, and may simplify the code in some ways. For example if we have code that changes the maximum supported range based on a config, it would probably be annoying to have to mutate the HashMap or use a different hash map if that config was on or off. I don't feel really strongly about the third point, but I suspect that the code will be messier if you try to remove features with supported version 0-0 from the internal data structures than if you don't. -- 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