cmccabe commented on a change in pull request #8737:
URL: https://github.com/apache/kafka/pull/8737#discussion_r432185717
##########
File path: core/src/main/scala/kafka/admin/TopicCommand.scala
##########
@@ -259,7 +263,8 @@ object TopicCommand extends Logging {
override def alterTopic(opts: TopicCommandOptions): Unit = {
val topic = new CommandTopicPartition(opts)
val topics = getTopics(opts.topic, opts.excludeInternalTopics)
- ensureTopicExists(topics, opts.topic)
+ ensureTopicExists(topics, opts.topic, !opts.ifExists)
Review comment:
I was wondering about how the tests passed! I guess I missed something
important about `getTopics`-- the fact that it returns either one or zero
topics. (I forgot that zero was a possibility).
The problem with how the code works today is that it's really very wasteful.
It's literally listing every topic in the cluster, (which could be tens of
thousands) just to see if the specific one you passed exists. There is also the
potential for a TOCTOU issue here (for example, it existed but then was deleted
right before you went to use it.)
So I would like to see this fixed to avoid doing the getTopics. I guess
think about it and if it seems reasonable to do it in the PR, let's go for it.
Otherwise I can create a follow-up JIRA.
We will need to fix stuff like this to scale the Kafka up. People listing
the full set of all topics is bad for scalability.
----------------------------------------------------------------
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:
us...@infra.apache.org
