rajinisivaram commented on a change in pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#discussion_r496173659
##########
File path:
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -1554,6 +1575,11 @@ private ConfigEntry configEntry(CreatableTopicConfigs
config) {
@Override
void handleFailure(Throwable throwable) {
+ // If there were any topics retries due to a quota exceeded
exception, we propagate
+ // the initial error back to the caller.
+ completeQuotaExceededException(futures,
quotaExceededExceptions,
Review comment:
Do we want to return quota exceeded in all cases? Apart from timeouts,
it seems like we should propagate failures rather an earlier quota exceeded
exception? That is, if we were throttled for 5 millis and then see a failure,
the failure is more useful than the fact that we were throttled for 5 millis?
##########
File path:
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java
##########
@@ -733,7 +733,9 @@ public void
testCreateTopicsRetryThrottlingExceptionWhenEnabledUntilRequestTimeO
time.sleep(defaultApiTimeout + 1);
assertNull(result.values().get("topic1").get());
- TestUtils.assertFutureThrows(result.values().get("topic2"),
TimeoutException.class);
+ ThrottlingQuotaExceededException e =
TestUtils.assertFutureThrows(result.values().get("topic2"),
+ ThrottlingQuotaExceededException.class);
+ assertEquals(0, e.throttleTimeMs());
Review comment:
I can see why we return the delta, but this looks odd when it says
throttled with a time of zero.
----------------------------------------------------------------
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
