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