-
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
feat: Pull Queries: QPS check utilizes internal API flag to determine if forwarded #5392
feat: Pull Queries: QPS check utilizes internal API flag to determine if forwarded #5392
Conversation
// treat it as having been forwarded. | ||
final boolean isAlreadyForwarded = routingOptions.skipForwardRequest() && | ||
// Trust the forward request option if isInternalRequest isn't available. | ||
isInternalRequest.orElse(true); |
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.
In which scenarios would isInternalRequest
not be available? Don't we want these two variables (skipForwardRequest
and isInternalRequest
) to always either both be true or both be false? Which makes me question whether we need both.
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.
isInternalRequest
wouldn't be available when the internal listener functionality is not enabled, so we still would want skipForwardRequest
in that case where it's running in a trusted setting and still wants to know to only forward the request once. But yes, you're right that they aim to infer the identical thing -- that this request is forwarded rather than from the end user.
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
Description
When doing pull queries, we do a QPS check. At the moment, we only check at the initial client request, regardless of whether it gets forwarded. This makes it easy to reason about the total cluster QPS.
To do this, we utilize a request property,
request.ksql.query.pull.skip.forwarding
. This PR adds the additional requirement that ifksql.internal.listener
is in use, the request must be internal to be considered a forwarded request.This prevents a user from setting
request.ksql.query.pull.skip.forwarding
explicitly.Testing done
mvn package
. Also tested manually.Reviewer checklist