Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-8670: Fix exception for kafka-topics.sh --describe without --topic mentioned #7094

Merged
merged 7 commits into from Jul 18, 2019

Conversation

@wyuka
Copy link
Contributor

commented Jul 15, 2019

If there are no topics in a cluster, kafka-topics.sh --describe without a --topic option should return empty list, not throw an exception.

We pass a boolean flag to ensureTopicExists method indicating whether to throw an exception if there are no topics in the cluster. In case of kafka-topics.sh --describe, the exception should NOT be thrown if either of these are true -

  1. A --topic option was not passed to the CLI. In that case, the output should be empty.
  2. A --if-exists option was passed to the CLI.
    Earlier, the first condition was not part of the check. This bugfix adds the first condition mentioned above to the check.

NOTE: I have added the desiredTopicName argument to the ensureTopicExists to check if it is defined. I could have simply passed !desiredTopicName.isDefined || topicOptWithExists in case of describe and it would still fix this bug. However, to fix bug KAFKA-8053, we need to improve the error message of the exception. The desiredTopicName will be required to build this exception message. I have not combined that messaging fix with this one to keep the commits for two bugs separate.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

I added the necessary unit test to check for this case.
Also, I ran these commands. The output before and after my change are mentioned inline

Describe without topic name - Before

./kafka-topics.sh --zookeeper 10.0.32.180:2181,10.0.16.211:2181,10.0.0.220:2181 --describe
Error while executing topic command : Topics in [] does not exist
[2019-07-15 23:23:25,674] ERROR java.lang.IllegalArgumentException: Topics in [] does not exist
	at kafka.admin.TopicCommand$.kafka$admin$TopicCommand$$ensureTopicExists(TopicCommand.scala:416)
	at kafka.admin.TopicCommand$ZookeeperTopicService.describeTopic(TopicCommand.scala:332)
	at kafka.admin.TopicCommand$.main(TopicCommand.scala:66)
	at kafka.admin.TopicCommand.main(TopicCommand.scala)
 (kafka.admin.TopicCommand$)

Describe without topic name - After (no output)

./kafka-topics.sh --zookeeper 10.0.32.180:2181,10.0.16.211:2181,10.0.0.220:2181 --describe

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)
wyuka added 2 commits Jul 15, 2019
KAFKA-8670 and KAFKA-8053: Fix kafka-topics.sh --describe without --t…
…opic mentioned if there are no topics in cluster, improve error message.

If there are no topics in a cluster, kafka-topics.sh --describe without
a --topic option should return empty list, do not throw an exception.

If there are no matching topics, throw IllegalArgumentException but with
better error message.

@wyuka wyuka changed the title KAFKA-8670 and KAFKA-8053: Fix kafka-topics.sh --describe without --t… KAFKA-8670: Fix kafka-topics.sh --describe without --topic mentioned if there are no topics in cluster Jul 16, 2019

@wyuka wyuka changed the title KAFKA-8670: Fix kafka-topics.sh --describe without --topic mentioned if there are no topics in cluster KAFKA-8670: Fix exception for kafka-topics.sh --describe without --topic mentioned Jul 16, 2019

@wyuka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

retest this please

@wyuka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Intermittent failures (not caused by this change) for two builds.

@wyuka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@hachikuji
Copy link
Contributor

left a comment

Thanks for the fix. Left a few small comments.

// If given topic doesn't exist then throw exception
throw new IllegalArgumentException(s"Topics in [${topics.mkString(",")}] does not exist")
if (desiredTopicName.isDefined) {

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 17, 2019

Contributor

Didn't we already check this?

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

Typo. Will remove.

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

Removed.

@@ -437,10 +437,13 @@ object TopicCommand extends Logging {
* @param topics
* @param topicOptWithExists
*/
private def ensureTopicExists(topics: Seq[String], topicOptWithExists: Boolean = false) = {
if (topics.isEmpty && !topicOptWithExists) {
private def ensureTopicExists(topics: Seq[String], desiredTopicName: Option[String], topicOptWithExists: Boolean = false) = {

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 17, 2019

Contributor

Might just be me, but I find this method a bit hard to understand. Maybe it is just a matter of choosing better names. Maybe something like this:

topics -> foundTopics
desiredTopicName -> requestedTopic
topicOptWithExists -> requireTopicExists

Then the check becomes:

requestedTopic.isDefined && requireTopicExists && foundTopics.isEmpty

That seems a little easier to understand. Maybe we can add a simple description for each parameter in the doc above.

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

That seems great. I will make these changes.

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

Changed variable names & added docs.

@@ -355,7 +355,7 @@ object TopicCommand extends Logging {
override def describeTopic(opts: TopicCommandOptions): Unit = {
val topics = getTopics(opts.topic, opts.excludeInternalTopics)
val topicOptWithExits = opts.topic.isDefined && opts.ifExists
ensureTopicExists(topics, topicOptWithExits)
ensureTopicExists(topics, opts.topic, topicOptWithExits)

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 17, 2019

Contributor

typo: should be topicOptWithExists

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

Yeah you are right. I did not rename the variable since it was already there. Let me do that.

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

Used requireTopicExists rather than topicOptWithExists. Felt like the previous one was not a very descriptive name.

@wyuka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

@hachikuji I addressed all the comments.

@hachikuji
Copy link
Contributor

left a comment

@wyuka Thanks for the update. Just one more minor comment and I'll merge.

// If given topic doesn't exist then throw exception
throw new IllegalArgumentException(s"Topics in [${topics.mkString(",")}] does not exist")
throw new IllegalArgumentException(s"Topics in [${foundTopics.mkString(",")}] does not exist")

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 17, 2019

Contributor

This message seems not very clear. We know foundTopics must be empty, so the message would look like this:

Topics in [] does not exist

I think it makes more sense for the message to indicate that the requested topic was not found. For example:

Topic 'foo' does not exist as expected

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

@hachikuji I know about this issue and had initially fixed this in this pull request. But I later realized that there is a separate JIRA https://issues.apache.org/jira/browse/KAFKA-8053 for it, and I can push a different commit for it. That is why I pushed a commit in this branch to revert the string change (225d6fd)

I am not aware of the general practice used for Apache Kafka, but assumed that we try to address separate bugs as separate commits, to improve traceability.

I guess in this case it makes sense to club them together, so I will make the string change again, and resolve the other issue when this is merged.

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 17, 2019

Contributor

Yeah, that's a good point. The two fixes largely overlap. So we can either fix both issues here or we can leave the fixed message for #6381. @scholzj Do you have any preference?

This comment has been minimized.

Copy link
@scholzj

scholzj Jul 17, 2019

Member

@hachikuji I don't have a strong preference. If you think its easier to fix it here in this PR and close mine I'm fine with it.

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 17, 2019

Author Contributor

Okay, @hachikuji I have made the string change here. We can resolve both JIRAs once this is merged.

@hachikuji
Copy link
Contributor

left a comment

Thanks, just one more comment.

@@ -354,8 +354,8 @@ object TopicCommand extends Logging {

override def describeTopic(opts: TopicCommandOptions): Unit = {
val topics = getTopics(opts.topic, opts.excludeInternalTopics)
val topicOptWithExits = opts.topic.isDefined && opts.ifExists
ensureTopicExists(topics, topicOptWithExits)
val requireTopicExists = !(opts.topic.isDefined && opts.ifExists)

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 18, 2019

Contributor

Initially I was going to post a comment on the redundant opts.topic.isDefined check here. As I was looking at this, however, I noticed that the second check seemed backwards. I think this has always been broken. We have the following documentation for the --if-exists argument:

if set when altering or deleting or describing topics, the action will only execute if the topic exists.

But we are actually raising the exception only when --if-exists is not defined (i.e. !topicOptWithExists in the original source). I think that is actually the root of the problem. So it seems we just do the following:

ensureTopicExists(topics, opts.topic, opts.ifExists)

Does that seem right?

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 18, 2019

Author Contributor

@hachikuji No, I don't think that's right. The --if-exists essentially means "check if the topic exists, and if it does go do this". Which means, we should not be throwing an exception if the topic does not exist. It is like performing a map() operation on an Optional in Java.

If this option is not passed, AND the topic does not exist, we should be throwing an exception.

We can actually simplify line 358 above to

ensureTopicExists(topics, opts.topic, !opts.ifExists)

Let me know your thoughts.

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 18, 2019

Contributor

Ok, that makes more sense. So the expected semantic is "do nothing if the topic doesn't exist." Probably the description could be improved. Anyway, the simplification sounds good.

This comment has been minimized.

Copy link
@wyuka

wyuka Jul 18, 2019

Author Contributor

@hachikuji Thanks. Updated with the simplification. The test servers seem to be having some issue connecting to github.com.

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 18, 2019

Contributor

Yeah, been seeing that lately. Not sure why.

By the way, I think I get why this logic seemed weird to me. If the topic doesn't exist, we still continue with the operation. But that doesn't make sense, right? Why go on deleting or altering the topic if it doesn't exist? Anyway, this can be improved separately.

@hachikuji
Copy link
Contributor

left a comment

Thanks for the patch! LGTM

@wyuka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@hachikuji What is the process for getting these merged into trunk (for 2.4.0) and for 2.3.1?

@hachikuji hachikuji merged commit ab8a7ff into apache:trunk Jul 18, 2019

2 of 3 checks passed

JDK 11 and Scala 2.12 FAILURE No test results found.
Details
JDK 11 and Scala 2.13 SUCCESS 11679 tests run, 67 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 11679 tests run, 67 skipped, 0 failed.
Details
hachikuji added a commit that referenced this pull request Jul 18, 2019
KAFKA-8670; Fix exception for kafka-topics.sh --describe without --to…
…pic mentioned (#7094)

If there are **no topics** in a cluster, kafka-topics.sh --describe without a --topic option should return empty list, not throw an exception.

Reviewers: Jason Gustafson <jason@confluent.io>
ijuma added a commit to confluentinc/kafka that referenced this pull request Jul 20, 2019
Merge remote-tracking branch 'apache-github/2.3' into ccs-2.3
* apache-github/2.3:
  MINOR: Update documentation for enabling optimizations (apache#7099)
  MINOR: Remove stale streams producer retry default docs. (apache#6844)
  KAFKA-8635; Skip client poll in Sender loop when no request is sent (apache#7085)
  KAFKA-8615: Change to track partition time breaks TimestampExtractor (apache#7054)
  KAFKA-8670; Fix exception for kafka-topics.sh --describe without --topic mentioned (apache#7094)
  KAFKA-8602: Separate PR for 2.3 branch (apache#7092)
  KAFKA-8530; Check for topic authorization errors in OffsetFetch response (apache#6928)
  KAFKA-8662; Fix producer metadata error handling and consumer manual assignment (apache#7086)
  KAFKA-8637: WriteBatch objects leak off-heap memory (apache#7050)
  KAFKA-8620: fix NPE due to race condition during shutdown while rebalancing (apache#7021)
  HOT FIX: close RocksDB objects in correct order (apache#7076)
  KAFKA-7157: Fix handling of nulls in TimestampConverter (apache#7070)
  KAFKA-6605: Fix NPE in Flatten when optional Struct is null (apache#5705)
  Fixes #8198 KStreams testing docs use non-existent method pipe (apache#6678)
  KAFKA-5998: fix checkpointableOffsets handling (apache#7030)
  KAFKA-8653; Default rebalance timeout to session timeout for JoinGroup v0 (apache#7072)
  KAFKA-8591; WorkerConfigTransformer NPE on connector configuration reloading (apache#6991)
  MINOR: add upgrade text (apache#7013)
  Bump version to 2.3.1-SNAPSHOT
xiowu0 added a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
[LI-CHERRY-PICK] [6f189d9] KAFKA-8670; Fix exception for kafka-topics…
….sh --describe without --topic mentioned (apache#7094)

TICKET = KAFKA-8670
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [6f189d9]
ORIGINAL_DESCRIPTION =

If there are **no topics** in a cluster, kafka-topics.sh --describe without a --topic option should return empty list, not throw an exception.

Reviewers: Jason Gustafson <jason@confluent.io>
(cherry picked from commit 6f189d9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.