[GitHub] [kafka] jsancio commented on a change in pull request #11711: MINOR: handle appending to configuration values that are empty better

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

jsancio commented on a change in pull request #11711:
URL: https://github.com/apache/kafka/pull/11711#discussion_r791893829



##########
File path: core/src/main/scala/kafka/server/ConfigAdminManager.scala
##########
@@ -515,4 +512,16 @@ object ConfigAdminManager {
       }
     }
   }
+
+  def getConfigPropertyAsList(
+    configProps: Properties,
+    configKeys: Map[String, ConfigKey],
+    configPropName: String
+  ): List[String] = {
+    Option(configProps.getProperty(configPropName))
+      .orElse(Option(ConfigDef.convertToString(configKeys(configPropName).defaultValue, ConfigDef.Type.LIST)))
+      .filter(_.trim.nonEmpty)
+      .map(_.split(",").toList)

Review comment:
       Do you think that we should trim and filter after splitting based on commas? For example should `"first , second, third"` be equivalent to `"first,second,third"`?

##########
File path: core/src/main/scala/kafka/server/ConfigAdminManager.scala
##########
@@ -495,10 +495,7 @@ object ConfigAdminManager {
         case OpType.APPEND => {
           if (!listType(alterConfigOp.configEntry.name, configKeys))
             throw new InvalidRequestException(s"Config value append is not allowed for config key: ${alterConfigOp.configEntry.name}")
-          val oldValueList = Option(configProps.getProperty(alterConfigOp.configEntry.name))
-            .orElse(Option(ConfigDef.convertToString(configKeys(configPropName).defaultValue, ConfigDef.Type.LIST)))
-            .getOrElse("")
-            .split(",").toList
+          val oldValueList = getConfigPropertyAsList(configProps, configKeys, configPropName)
           val newValueList = oldValueList ::: alterConfigOp.configEntry.value.split(",").toList

Review comment:
       Do you think that we should also `trim` the value that is getting appended? If I understand this code correctly, Kafka will store the  white space but a future `APPEND` will trim them.




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