You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/12/07 23:37:16 UTC
[GitHub] [kafka] cmccabe opened a new pull request #11577: KAFKA-13515: Fix KRaft config validation issues
cmccabe opened a new pull request #11577:
URL: https://github.com/apache/kafka/pull/11577
Require that topics exist before topic configurations can be created for them.
Merge the code from ConfigurationControlManager#checkConfigResource into
ControllerConfigurationValidator to avoid duplication.
Add KRaft support to DynamicConfigChangeTest.
Split out tests in DynamicConfigChangeTest that don't require a cluster into
DynamicConfigChangeUnitTest to save test time.
--
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
[GitHub] [kafka] cmccabe merged pull request #11577: KAFKA-13515: Fix KRaft config validation issues
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #11577:
URL: https://github.com/apache/kafka/pull/11577
--
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
[GitHub] [kafka] cmccabe merged pull request #11577: KAFKA-13515: Fix KRaft config validation issues
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #11577:
URL: https://github.com/apache/kafka/pull/11577
--
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
[GitHub] [kafka] mumrah commented on a change in pull request #11577: KAFKA-13515: Fix KRaft config validation issues
Posted by 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
[GitHub] [kafka] cmccabe commented on a change in pull request #11577: KAFKA-13515: Fix KRaft config validation issues
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11577:
URL: https://github.com/apache/kafka/pull/11577#discussion_r765472041
##########
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:
Hmm... I'm not aware of any. `ZkAdminManager` just uses `Integer.valueOf`, as does `TopicCommand` and `KafkaConfig`.
--
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
[GitHub] [kafka] cmccabe commented on a change in pull request #11577: KAFKA-13515: Fix KRaft config validation issues
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11577:
URL: https://github.com/apache/kafka/pull/11577#discussion_r765472880
##########
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:
ok
--
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
[GitHub] [kafka] cmccabe commented on a change in pull request #11577: KAFKA-13515: Fix KRaft config validation issues
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11577:
URL: https://github.com/apache/kafka/pull/11577#discussion_r765467726
##########
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.
Sounds good, will add.
> 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
I did do that for a bunch of other controller code, but over time I've been converting it to use exceptions. The problem is that there are a lot of places where exceptions can be thrown, so you always end up setting up a catch block anyway. If there's no catch block there will inevitably be a bug where something is thrown and not caught.
I wish we were using a language like Rust or Go where exceptions were reserved for logic errors, but it's unfortunately not the case. So, when in Rome...
--
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