dajac commented on a change in pull request #9334:
URL: https://github.com/apache/kafka/pull/9334#discussion_r494810769
##########
File path:
core/src/test/scala/unit/kafka/admin/TopicCommandWithAdminClientTest.scala
##########
@@ -844,4 +851,72 @@ class TopicCommandWithAdminClientTest extends
KafkaServerTestHarness with Loggin
assertEquals(2, rows.size)
rows(0).startsWith(s"Topic:$testTopicName\tPartitionCount:1")
}
+
+ @Test
+ def testCreateTopicDoesNotRetryThrottlingQuotaExceededException(): Unit = {
+ val adminClient = Mockito.mock(classOf[Admin])
+ val topicService = AdminClientTopicService(adminClient)
+
+ val result = AdminClientTestUtils.createTopicsResult(testTopicName,
Errors.THROTTLING_QUOTA_EXCEEDED.exception())
+ Mockito.when(adminClient.createTopics(ArgumentMatchers.any(),
ArgumentMatchers.any())).thenReturn(result)
Review comment:
That makes sense. I had to introduce an alias for `ArgumentMatchers.eq`
to not conflict with `eq`. I went with `eqThat` to remain inline with `argThat`.
##########
File path: core/src/main/scala/kafka/admin/TopicCommand.scala
##########
@@ -69,16 +71,26 @@ object TopicCommand extends Logging {
else if (opts.hasDeleteOption)
topicService.deleteTopic(opts)
} catch {
+ case e: ExecutionException =>
+ if (e.getCause != null)
+ printException(e.getCause)
+ else
+ printException(e)
+ exitCode = 1
case e: Throwable =>
- println("Error while executing topic command : " + e.getMessage)
- error(Utils.stackTrace(e))
+ printException(e)
exitCode = 1
} finally {
topicService.close()
Exit.exit(exitCode)
}
}
+ private def printException(e: Throwable): Unit = {
+ println("Error while executing topic command : " + e.getMessage)
+ error(Utils.stackTrace(e))
Review comment:
Do you mean using `error(message, e)` to replace both `println` and
`error`? I think that we are using 'println` here in order to print the message
to stdout without any logger related stuff and regardless of how the logger is
configured. Changing to using `error(message, e)` would break this and
potentially break existing application due to introducing the logger related
stuff for that message. I think that we should keep `println` here.
However, I wonder if using `error` is appropriate here as it basically
prints a stacktrace for every errors. The UX does not look good. Would it make
sense to use `debug` instead? The message of the exception is printed anyway
and I don't think that the stacktrace provides much to regular users. WDYT?
##########
File path: core/src/main/scala/kafka/admin/TopicCommand.scala
##########
@@ -69,16 +71,26 @@ object TopicCommand extends Logging {
else if (opts.hasDeleteOption)
topicService.deleteTopic(opts)
} catch {
+ case e: ExecutionException =>
+ if (e.getCause != null)
+ printException(e.getCause)
+ else
+ printException(e)
+ exitCode = 1
case e: Throwable =>
- println("Error while executing topic command : " + e.getMessage)
- error(Utils.stackTrace(e))
+ printException(e)
exitCode = 1
} finally {
topicService.close()
Exit.exit(exitCode)
}
}
+ private def printException(e: Throwable): Unit = {
+ println("Error while executing topic command : " + e.getMessage)
+ error(Utils.stackTrace(e))
Review comment:
That's a very good question. I guess that we did so in order to just
print out the stacktrace without the message. Otherwise, we would have the
error message printed out by the `println`, followed by the same message
printed out by logger, followed by the stacktrace. Having the message twice is
not necessary. I had a quick look at other commands and we do so everywhere.
I will open a JIRA to make tools more consistent. Good idea.
----------------------------------------------------------------
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
