-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix: Always dec. concurrency counter, even hitting rate limits #8165
fix: Always dec. concurrency counter, even hitting rate limits #8165
Conversation
0a561ef
to
704b30b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @AlanConfluent. Thanks for the fix! Couple of minor comments
@@ -316,7 +316,7 @@ private EndpointResponse handleStatement( | |||
} | |||
} catch (final TopicAuthorizationException e) { | |||
return errorHandler.accessDeniedFromKafkaResponse(e); | |||
} catch (final KsqlStatementException e) { | |||
} catch (final KsqlStatementException e) {; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mistakenly added ;
?
@@ -93,6 +96,7 @@ protected synchronized void allow(final KsqlQueryType ksqlQueryType, final long | |||
this.numBytesInWindow -= responseSizesLog.poll().right; | |||
} | |||
if (this.numBytesInWindow > throttleLimit) { | |||
LOG.warn("Hit bandwidth rate limit of " + throttleLimit + " with use of " + numBytesInWindow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add the unit MB
in the warning message here to be more explicit
@AlanConfluent Do we foresee the concurrency counter not getting decremented when we hit other types of exceptions in pull query execution? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AlanConfluent ! This is a great catch.
I just had one question.
} catch (final Throwable t) { | ||
LOG.error("Error closing pull query queue", t); | ||
} | ||
future.complete(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be completed exceptionally if we fail to close?
61492ce
to
0dd01b6
Compare
* fix: Always dec. concurrency counter, even hitting rate limits
…uentinc#8165) * fix: Always dec. concurrency counter, even hitting rate limits
#8223) * fix: Always dec. concurrency counter, even hitting rate limits (#8165) * fix: Always dec. concurrency counter, even hitting rate limits * chore: fix pull query compilation Co-authored-by: Alan Sheinberg <[email protected]>
Description
When the bandwidth rate limit is hit, it never decrements the concurrency counter. This change ensures it always does.
Testing done
Ran tests and verified manually
Reviewer checklist