[GitHub] [kafka] ivanyu commented on a change in pull request #9990: KAFKA-12235: Fix ZkAdminManager.describeConfigs on 2+ config keys

[GitBox <gi...@apache.org>]

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?




----------------------------------------------------------------
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:
users@infra.apache.org