[GitHub] [kafka] dengziming commented on a diff in pull request #12199: KAFKA-13609: Use INVALID_CONFIG when config value is null or broker config validation fails

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

dengziming commented on code in PR #12199:
URL: https://github.com/apache/kafka/pull/12199#discussion_r880354039


##########
core/src/main/scala/kafka/server/ConfigAdminManager.scala:
##########
@@ -200,11 +200,7 @@ class ConfigAdminManager(nodeId: Int,
       conf.dynamicConfig.validate(props, !configResource.name().isEmpty)
     } catch {
       case e: ApiException => throw e
-      //KAFKA-13609: InvalidRequestException is not really the right exception here if the
-      // configuration fails validation. The configuration is still well-formed, but just
-      // can't be applied. It should probably throw InvalidConfigurationException. However,
-      // we should probably only change this in a KIP since it has compatibility implications.

Review Comment:
   Thanks for reminding this, we already discussed this in #12162, currently, we would throw `ConfigException` in `DynamicBrokerConfig.validate()` and it will be converted to `InvalidConfigurationException` finally:
   https://github.com/apache/kafka/blob/9dc332f5ca34b80af369646f767c40c6b189f831/core/src/main/scala/kafka/server/ZkAdminManager.scala#L410
   
   since we already raise `InvalidConfigurationException` in some cases and it was documented explicitly in related KIPs,  it's unnecessary for us to create another KIP for this change.



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