[GitHub] [kafka] dajac commented on a change in pull request #8984: KAFKA-10227: Enforce cleanup policy to only contain compact or delete once

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

dajac commented on a change in pull request #8984:
URL: https://github.com/apache/kafka/pull/8984#discussion_r450315712



##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -268,6 +268,17 @@ object ConfigCommand extends Config {
       println(s"WARNING: The configuration ${LogConfig.MessageFormatVersionProp}=${props.getProperty(LogConfig.MessageFormatVersionProp)} is specified. " +
         s"This configuration will be ignored if the version is newer than the inter.broker.protocol.version specified in the broker.")
     }
+    props.forEach((config, value) => {
+      if (value.toString.contains(",")) {

Review comment:
       It seems a bit dangerous to treat all the values containing a `,` as lists that can be de-duplicated. We may have json encoded values or other specific formats in the futures. KIP-574 has even made some improvements for this: https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input.
   
   Wouldn't it be better to do this only for explicit cases (e.g. certain keys known to be lists)? One way could be the leverage the `ConfigDef` used to validate the configuration. At the moment, the `LIST` accepts duplicates but we could improve this by either altering the implementation of introducing a new `SET` types for instance.




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