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/09/28 11:27:31 UTC
[GitHub] [kafka] dajac opened a new pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
dajac opened a new pull request #9344:
URL: https://github.com/apache/kafka/pull/9344
KIP-599 had proposed to keep returning the `ThrottlingQuotaExceededException` to the called even when the request times out due to reaching `default.api.timeout.ms`. The current implementation does not cover this yet.
From KIP-599:
> Once `default.api.timeout.ms` has been reached, the topics which were throttled will return the `ThrottlingQuotaExceededException` to the caller.
This PR adds the logic to preserve the `ThrottlingQuotaExceededException` when topics are retried. The `throttleTimeMs` is also adjusted accordingly as the request could remain pending or in-flight for quite a long time.
I have run various tests on clusters with enabled quotas and I, indeed, find it better to preserve the exception. Otherwise, the caller does not really understand what is going on. This allows the caller to take the appropriate measure and also to take the `throttleTimeMs` into consideration.
### 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] dajac commented on a change in pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#discussion_r496645598
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -1554,6 +1575,13 @@ 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 if the request timed out.
+ if (options.shouldRetryOnQuotaViolation() && throwable instanceof TimeoutException) {
+ completeQuotaExceededException(futures, quotaExceededExceptions,
Review comment:
That's a very good suggestion, thanks.
----------------------------------------------------------------
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] rajinisivaram commented on pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#issuecomment-700735225
Builds are good, merging to trunk.
----------------------------------------------------------------
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] rajinisivaram commented on a change in pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on a change in pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#discussion_r496579003
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -1554,6 +1575,13 @@ 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 if the request timed out.
+ if (options.shouldRetryOnQuotaViolation() && throwable instanceof TimeoutException) {
+ completeQuotaExceededException(futures, quotaExceededExceptions,
Review comment:
Perhaps we could make this `maybeCompleteQuotaExceededException` and pass in the throwable, so that we can do the check for TimeoutException in that helper method rather than in every `handleFailure`?
----------------------------------------------------------------
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] dajac commented on a change in pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#discussion_r496473270
##########
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:
Yeah, I do agree but this could happen even if this should be rare. This test case is a bit stretched to verify that throttle time does not go below zero.
The reasoning of doing this is that a client could be throttled for longer than `default.api.timeout.ms`. When this happens, I believe that we should return an adjusted throttle time such that the client does not have to re-wait for the time that it has already waited for.
----------------------------------------------------------------
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] dajac commented on pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#issuecomment-700557270
@rajinisivaram Thanks for your comments. I have updated 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] dajac commented on a change in pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#discussion_r496467056
##########
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:
Definitely. I meant to do it for `TimeoutException` only but I have forgotten it while implementing it :(. Let me correct this.
----------------------------------------------------------------
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] rajinisivaram merged pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
rajinisivaram merged pull request #9344:
URL: https://github.com/apache/kafka/pull/9344
----------------------------------------------------------------
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] dajac commented on pull request #9344: MINOR; Preserve ThrottlingQuotaExceededException when request timeouts after being retried due to a quota violation (KIP-599)
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9344:
URL: https://github.com/apache/kafka/pull/9344#issuecomment-700644563
@rajinisivaram Thanks for your suggestion. I have updated 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