ivanyu commented on a change in pull request #9990: URL: https://github.com/apache/kafka/pull/9990#discussion_r582559897
########## File path: core/src/main/scala/kafka/server/ConfigHelper.scala ########## @@ -47,11 +47,11 @@ class ConfigHelper(metadataCache: MetadataCache, config: KafkaConfig, configRepo def createResponseConfig(configs: Map[String, Any], createConfigEntry: (String, Any) => DescribeConfigsResponseData.DescribeConfigsResourceResult): DescribeConfigsResponseData.DescribeConfigsResult = { - val filteredConfigPairs = if (resource.configurationKeys == null) + val filteredConfigPairs = if (resource.configurationKeys == null || resource.configurationKeys.isEmpty) Review comment: I agree with this. However it seems treating empty as null is de facto behavior at the moment (before the fix). To be more precise, here https://github.com/apache/kafka/blob/e2a0d0c90e1916d77223a420e3595e8aba643001/core/src/main/scala/kafka/server/ConfigHelper.scala#L53-L55 `resource.configurationKeys` being empty means `true` for each element of `configs`, so effectively no filtering. Which, in turn, is equivalent to `resource.configurationKeys == null`. For example, `testDescribeConfigsWithDocumentation` and `testDescribeConfigsWithEmptyConfigurationKeys` in `ZkAdminManagerTest` break after the fix if treating empty as null is not kept. I guess it originates from that the default constructor of `DescribeConfigsResource` makes `configurationKeys` an empty array. We can either 1) keep the current behavior (the current approach); or 2) stick to the spec (also potentially adding `"default": "null"` to the field description). I'd prefer 2. What do you think, @cmccabe? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org