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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
