-
Notifications
You must be signed in to change notification settings - Fork 592
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
CORE-2699 Quotas: prepare for using the quota store in client_quota_translator
#18777
Merged
pgellert
merged 6 commits into
redpanda-data:dev
from
pgellert:quotas/integrate-quota-store-translator-basics
Jun 5, 2024
Merged
CORE-2699 Quotas: prepare for using the quota store in client_quota_translator
#18777
pgellert
merged 6 commits into
redpanda-data:dev
from
pgellert:quotas/integrate-quota-store-translator-basics
Jun 5, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pgellert
requested review from
graphcareful and
BenPope
and removed request for
a team
June 4, 2024 19:47
BenPope
reviewed
Jun 5, 2024
We should start each test with clean configs to prevent them from interfering with eachother.
A follow up commit adds kafka/types.h as a transitive dependency to the client_quota_translator_test.cc which causes the client_id constant in the test file to clash with the client_id type alias from types.h.
This fixes a bug in the quota values being returned for group-based quota keys where we would incorrectly fall back to the default values for group-based keys when the correct quota is really "not applicable" (i.e. `std::nullopt`). This is not an observable bug because these quota values are never really accessed. If the request had a group-based applicable quota, it would both have a group-based key and its limit. It's not possible for a request to have a group-based key and a default quota value. We fix this bug now because we are about to integrate with the quota store, which makes this bug more apparent.
This wires the sharded quota_store into the client_quota_translator. There's no behaviour change yet (the quota store is not accessed yet), so this is merely a refactor.
This refactors the quota lookup methods to be generic across the different request types in a way that allows us to hook in quotas from the quota store in follow up commits. Refactor only, no behaviour change.
pgellert
force-pushed
the
quotas/integrate-quota-store-translator-basics
branch
from
June 5, 2024 13:27
ab535f3
to
c1b933d
Compare
Force-pushed to address Ben's feedback and to get the |
7 tasks
BenPope
approved these changes
Jun 5, 2024
graphcareful
approved these changes
Jun 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A set of fixup commits and refactors that lead up to the next PR which will start using the
client_quota::store
insideclient_quota_translator
. These commits are extracted to aid review.Backports Required
Release Notes