divijvaidya commented on code in PR #14176:
URL: https://github.com/apache/kafka/pull/14176#discussion_r1291232121
##########
core/src/main/scala/kafka/server/ControllerServer.scala:
##########
@@ -231,7 +231,7 @@ class ControllerServer(
setMetrics(quorumControllerMetrics).
setCreateTopicPolicy(createTopicPolicy.asJava).
setAlterConfigPolicy(alterConfigPolicy.asJava).
- setConfigurationValidator(new ControllerConfigurationValidator()).
+ setConfigurationValidator(new
ControllerConfigurationValidator(sharedServer.brokerConfig)).
Review Comment:
`sharedServer` represents the case when kraft controller is run on a broker
and not as an independent node. In such case a broker has two responsibility,
one to act as a controller and another to act as a broker. These two configs
represent the configurations associated with node's role as a broker and as a
controller.
In our case here, when createTopic or alterConfig is called to enable TS for
a topic, it will be forwarded to the controller. Controller will validate the
config using `ControllerConfigurationValidator` before applying it. The
assumption here is that the broker level configuration has already been
validated before forwarding. Now, this is the first case where we want to make
an assertion consisting of both broker level configuration and topic level
configuration. I would ideally have wanted to fail fast with this at broker
itself before it is sent to controller by adding the validation at
https://github.com/apache/kafka/blob/f137da04fa71734d176e19f5800622f4b4dfdb66/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L155
Perhaps, you can make a change there?
--
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]
