[GitHub] [kafka] mumrah commented on a change in pull request #11577: KAFKA-13515: Fix KRaft config validation issues

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

mumrah commented on a change in pull request #11577:
URL: https://github.com/apache/kafka/pull/11577#discussion_r765181618



##########
File path: metadata/src/main/java/org/apache/kafka/controller/QuorumController.java
##########
@@ -1015,6 +1048,11 @@ private void resetState() {
      */
     private final ControllerPurgatory purgatory;
 
+    /**
+     * A predicate that returns information about whether a ConfigResource exists.
+     */
+    private final ConfigResourceExistenceChecker resourceExists;

Review comment:
       nit: maybe use the less specific type here? 

##########
File path: metadata/src/main/java/org/apache/kafka/controller/QuorumController.java
##########
@@ -251,6 +254,36 @@ public QuorumController build() throws Exception {
         }
     }
 
+    class ConfigResourceExistenceChecker implements Consumer<ConfigResource> {

Review comment:
       We should add a comment that this thing should only be called by the event queue thread.

##########
File path: metadata/src/main/java/org/apache/kafka/controller/QuorumController.java
##########
@@ -251,6 +254,36 @@ public QuorumController build() throws Exception {
         }
     }
 
+    class ConfigResourceExistenceChecker implements Consumer<ConfigResource> {

Review comment:
       Also, it might be nicer in terms of control flow to return an ApiError instance instead of throwing exceptions. Since these are all unchecked errors, the caller has to know to capture one and convert it to an ApiError

##########
File path: core/src/main/scala/kafka/server/ControllerConfigurationValidator.scala
##########
@@ -47,7 +52,17 @@ class ControllerConfigurationValidator extends ConfigurationValidator {
         }
         LogConfig.validate(properties)
       case BROKER =>
-        // TODO: add broker configuration validation
+        if (resource.name().nonEmpty) {

Review comment:
       Seems like we probably have other places where we validate broker IDs from strings. Is there a utility method we can use here? 




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