Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
KAFKA-13515: Fix KRaft config validation issues #11577
Conversation
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.
| } | ||
| } | ||
|
|
||
| class ConfigResourceExistenceChecker implements Consumer<ConfigResource> { |
There was a problem hiding this comment.
We should add a comment that this thing should only be called by the event queue thread.
There was a problem hiding this 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
There was a problem hiding this 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...
| /** | ||
| * A predicate that returns information about whether a ConfigResource exists. | ||
| */ | ||
| private final ConfigResourceExistenceChecker resourceExists; |
There was a problem hiding this comment.
nit: maybe use the less specific type here?
| LogConfig.validate(properties) | ||
| case BROKER => | ||
| // TODO: add broker configuration validation | ||
| if (resource.name().nonEmpty) { |
There was a problem hiding this comment.
Seems like we probably have other places where we validate broker IDs from strings. Is there a utility method we can use here?
There was a problem hiding this comment.
Hmm... I'm not aware of any. ZkAdminManager just uses Integer.valueOf, as does TopicCommand and KafkaConfig.
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. Reviewers: David Arthur <mumrah@gmail.com>
2 participants
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.