HBASE-27784: support quota user overrides #5424
Merged
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
rmdmattingly
commented
Sep 20, 2023
...-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaUserOverrideConfiguration.java
Outdated
Show resolved
Hide resolved
Comment on lines
65
to
68
| // defines the request attribute key which, when provided, will override the request's username | ||
| // from the perspective of user quotas. This is also the default request attribute key | ||
| public static final String QUOTA_USER_REQUEST_ATTRIBUTE_OVERRIDE_KEY = | ||
| "hbase.quota.user.override.key"; |
There was a problem hiding this comment.
The main advantage of making this configurable is so that users can make use of existing attributes. For example we already submit an "upstream caller" identifier in our request attributes so that they will manifest in the slow query logs (introduced here). It would be nice to make use of that attribute here rather than requiring that we send superfluous bytes over the wire for each request
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27784-draft
branch
from
7c99721 to
ac13213
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
The test failures above look unrelated |
ndimiduk
approved these changes
Sep 22, 2023
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaUserOverride.java
Outdated
Show resolved
Hide resolved
...-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaUserOverrideConfiguration.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaUserOverride.java
Outdated
Show resolved
Hide resolved
| assertEquals(10, doPuts(10, FAMILY, QUALIFIER, tableWithoutThrottle)); | ||
|
|
||
| // Remove all the limits | ||
| admin.setQuota(QuotaSettingsFactory.unthrottleUser(userOverrideWithQuota)); |
There was a problem hiding this comment.
testing that removing the limits dynamically is honored -- nice.
Yeah I've noticed that |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| Optional<RpcCall> rpcCall = RpcServer.getCurrentCall(); | ||
| if ( | ||
| rpcCall.isPresent() | ||
| && rpcCall.get().getRequestAttributes().containsKey(userOverrideRequestAttributeKey) |
There was a problem hiding this comment.
I might recommend just looping the raw rpcCall.get().getHeader().getAttributes() here. For a small N (as i expect attributes to be), it is typically faster to iterate the list than to lookup in a map. This is especially true if we have to actually build the map as well (as we are likely to here). So in a vacuum, this call loops all the attributes and populates a map (which involves hashcode and equals for each, not to mention converting from ByteString to byte[] and string). Then we do a lookup, which involves another hashcode and equals. On the other hand, a loop can exit out early if a match is found and only requires equals.
So if we planned to do many operations on the map, it might start to pay off to do those conversions. But if we are just looking for a single key in an empty or very small list, it'd be cheaper to skip all of that.
There was a problem hiding this comment.
yeah that makes sense, seems like a reasonable optimization. should we bake that into an RpcCall#getRequestAttribute(String key) method to help this pattern be standard or is that overkill?
There was a problem hiding this comment.
Yea, let's do that. We can have it pull from the map if it's non-null, and otherwise loop. Just add javadoc describing the difference between that and getRequestAttributes() and when to use each.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Sep 27, 2023
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
pushed a commit
that referenced
this pull request
Sep 27, 2023
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault
pushed a commit
that referenced
this pull request
Sep 27, 2023
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
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.
Description
At my day job we struggle to use quotas effectively for our most important HBase users. This is because our most important users are often proxy APIs which are queried by countless "upstream" microservices. It isn't feasible for these proxy APIs to spin up new connections for each request, so it isn't feasible to have distinct UserGroupInformation at request granularity. This leaves us unable to throttle these upstream microservices in isolation.
In #5326 we added support for "request attributes" — these are like operation attributes, except they exist 1:1 with RpcCalls rather than 1:1 with operations. In this PR we build on the request attribute framework and introduce the ability to specify a "quota user override" so that one can apply separate throttles to many requests sent by a single shared Connection.
Usage
For example, one may add the following property to their HBase configuration:
One could then create a Table with a quota user override via the TableBuilder:
cc @bbeaudreault @hgromer @eab148 @bozzkar