KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config #8717
Conversation
|
@cmccabe this is ready for review. The test location doesn't quite fit as it's in the module test dynamic config changes on the broker, but it's "near" the old error. Any ideas on where it might be better situated? |
| @@ -454,6 +454,9 @@ class AdminManager(val config: KafkaConfig, | |||
| private def alterTopicConfigs(resource: ConfigResource, validateOnly: Boolean, | |||
| configProps: Properties, configEntriesMap: Map[String, String]): (ConfigResource, ApiError) = { | |||
| val topic = resource.name | |||
| if (!metadataCache.contains(topic)) | |||
| throw new UnknownTopicOrPartitionException(s"The topic '$topic' does not exist.") | |||
|
|
|||
| adminZkClient.validateTopicConfig(topic, configProps) | |||
There was a problem hiding this comment.
It seems not all checks in validateTopicConfig are required.
Topic.validate(topic)-> the topic is validated already as the topic is createdif (!zkClient.topicExists(topic))-> it is replaced byif (!metadataCache.contains(topic))LogConfig.validate(configs)-> this should be reserved
In short, should we remove adminZkClient.validateTopicConfig(topic, configProps) and then add LogConfig.validate(configs)?
There was a problem hiding this comment.
hmmm, the following changeTopicConfig checks the topic name and configs again. Not sure why there are such duplicate checks.
There was a problem hiding this comment.
yeah, there is lot scope for cleanup. we can improve in future PRs.
There was a problem hiding this comment.
Thanks for the PR @bdbyrne, I'm curious why we don't consider fixing the AdminZkClient#validateTopicConfig thrown exception type, but fix its caller instead. And in fact we have two callers for that path as well, so maybe a more comprehensive fix is to fix AdminZkClient IIUC.
Hi @abbccdda -- there is a fix for the |
|
@bdbyrne Can you rebase the PR? |
…n-existent topic config.
|
ok to test |
|
retest this please |
1 similar comment
|
retest this please |
|
retest this please |
1 similar comment
|
retest this please |
|
ok to test |
Committer Checklist (excluded from commit message)