[GitHub] [kafka] rajinisivaram commented on a change in pull request #8723: KIP-569-KAFKA-9494: DescribeConfigsResponse - include additional metadata information

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

rajinisivaram commented on a change in pull request #8723:
URL: https://github.com/apache/kafka/pull/8723#discussion_r431870218



##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -2052,6 +2057,49 @@ void handleFailure(Throwable throwable) {
         return configSource;
     }
 
+    private ConfigEntry.ConfigType configType(DescribeConfigsResponse.ConfigType type) {
+        if (type == null) {
+            return ConfigEntry.ConfigType.UNKNOWN;
+        }
+
+        ConfigEntry.ConfigType configType;
+        switch (type) {
+            case BOOLEAN:
+                configType = ConfigEntry.ConfigType.BOOLEAN;
+                break;
+            case CLASS:
+                configType = ConfigEntry.ConfigType.CLASS;
+                break;
+            case DOUBLE:
+                configType = ConfigEntry.ConfigType.DOUBLE;
+                break;
+            case INT:
+                configType = ConfigEntry.ConfigType.INT;
+                break;
+            case LIST:
+                configType = ConfigEntry.ConfigType.LIST;
+                break;
+            case LONG:
+                configType = ConfigEntry.ConfigType.LONG;
+                break;
+            case PASSWORD:
+                configType = ConfigEntry.ConfigType.PASSWORD;
+                break;
+            case SHORT:
+                configType = ConfigEntry.ConfigType.SHORT;
+                break;
+            case STRING:
+                configType = ConfigEntry.ConfigType.STRING;
+                break;
+            case UNKNOWN:
+                configType = ConfigEntry.ConfigType.UNKNOWN;
+                break;
+            default:
+                throw new IllegalArgumentException("Unexpected config type " + type);

Review comment:
       Should this return `UNKNOWN` since there may be a type in a new version that the client doesn't know about?

##########
File path: clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -201,6 +201,13 @@ public String getString(String key) {
         return configKey.type;
     }
 
+    public String documentationOf(String key) {

Review comment:
       Since this is a public API, we should include this change in the KIP.

##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DescribeConfigsRequest.java
##########
@@ -43,6 +43,7 @@
     private static final String RESOURCE_TYPE_KEY_NAME = "resource_type";
     private static final String RESOURCE_NAME_KEY_NAME = "resource_name";
     private static final String CONFIG_NAMES_KEY_NAME = "config_names";
+    private static final String INCLUDE_DOCUMENTATION = "include_documentation";

Review comment:
       Can we switch over to using the generated `DescribeConfigsRequestData` class?

##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -347,7 +347,7 @@ class AdminManager(val config: KafkaConfig,
     }
   }
 
-  def describeConfigs(resourceToConfigNames: Map[ConfigResource, Option[Set[String]]], includeSynonyms: Boolean): Map[ConfigResource, DescribeConfigsResponse.Config] = {
+    def describeConfigs(resourceToConfigNames: Map[ConfigResource, Option[Set[String]]], includeSynonyms: Boolean, includeDocumentation: Boolean): Map[ConfigResource, DescribeConfigsResponse.Config] = {

Review comment:
       indentation

##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DescribeConfigsResponse.java
##########
@@ -56,6 +56,8 @@
     private static final String IS_SENSITIVE_KEY_NAME = "is_sensitive";
     private static final String IS_DEFAULT_KEY_NAME = "is_default";
     private static final String READ_ONLY_KEY_NAME = "read_only";
+    private static final String CONFIG_VALUE_TYPE_KEY_NAME = "config_value_type";
+    private static final String CONFIG_DOCUMENTATION_KEY_NAME = "config_documentation";

Review comment:
       Same as the request, can we switch over to the generated class?

##########
File path: clients/src/main/resources/common/message/DescribeConfigsRequest.json
##########
@@ -32,6 +32,8 @@
         "about": "The configuration keys to list, or null to list all configuration keys." }
     ]},
     { "name": "IncludeSynoyms", "type": "bool", "versions": "1+", "default": "false", "ignorable": false,

Review comment:
       Unrelated to this PR, but since you are updating this file anyway, can you fix the typo in `IncludeSynonyms`?




----------------------------------------------------------------
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