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 2020/05/22 16:49:12 UTC
[GitHub] [kafka] bdbyrne opened a new pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
bdbyrne opened a new pull request #8717:
URL: https://github.com/apache/kafka/pull/8717
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-640014947
ok to test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639036963
@bdbyrne Can you rebase the PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy removed a comment on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy removed a comment on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639344362
KAFKA-9838
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#discussion_r429551641
##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -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)
Review comment:
It seems not all checks in ```validateTopicConfig``` are required.
1. ```Topic.validate(topic)``` -> the topic is validated already as the topic is created
1. ```if (!zkClient.topicExists(topic))``` -> it is replaced by ```if (!metadataCache.contains(topic))```
1. ```LogConfig.validate(configs)``` -> this should be reserved
In short, should we remove ```adminZkClient.validateTopicConfig(topic, configProps)``` and then add ```LogConfig.validate(configs)```?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on a change in pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on a change in pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#discussion_r435468757
##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -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)
Review comment:
yeah, there is lot scope for cleanup. we can improve future PRs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-632799858
@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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne edited a comment on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
bdbyrne edited a comment on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-637016443
> 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 `AdminZkClient` at https://github.com/apache/kafka/pull/8715. This PR can be seen as an alternative/supplement in the case that changing the exception has other ramifications. I'm fine with dropping this if you feel the other PR is better.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639336059
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639307479
ok to test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639344362
KAFKA-9838
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on a change in pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on a change in pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#discussion_r435468757
##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -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)
Review comment:
yeah, there is lot scope for cleanup. we can improve in future PRs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639979627
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639816965
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-639641282
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#discussion_r429552092
##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -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)
Review comment:
hmmm, the following ```changeTopicConfig``` checks the topic name and configs again. Not sure why there are such duplicate checks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne commented on pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8717:
URL: https://github.com/apache/kafka/pull/8717#issuecomment-637016443
> 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 Boyang -- there is a fix for the `AdminZkClient` at https://github.com/apache/kafka/pull/8715. This PR can be seen as an alternative/supplement in the case that changing the exception has other ramifications. I'm fine with dropping this if you feel the other PR is better.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy closed pull request #8717: KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a non-existent topic's config
Posted by GitBox <gi...@apache.org>.
omkreddy closed pull request #8717:
URL: https://github.com/apache/kafka/pull/8717
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org