HBASE-27687: support consumption of block bytes scanned in operation quota
#5654
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
Jan 26, 2024
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/DefaultOperationQuota.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.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. |
|
🎊 +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-27687
branch
from
dd7adca to
e7c8a05
Compare
|
Build number 2 failed above sort of inexplicably AFAICT, so the unit test report is not available. I believe I've fixed the legitimate unit test failures from build number 1. I've force pushed to restart the build process. |
|
🎊 +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. |
|
The test failure here appears to be unrelated and transient |
bbeaudreault
reviewed
Feb 2, 2024
| @@ -690,6 +690,7 @@ private Result increment(final HRegion region, final OperationQuota quota, | |||
| if (metricsRegionServer != null) { | |||
| long blockBytesScanned = | |||
| context != null ? context.getBlockBytesScanned() - blockBytesScannedBefore : 0; | |||
| quota.addBlockBytesScanned(blockBytesScanned); | |||
There was a problem hiding this comment.
this may not be a huge issue, but i think this shouldn't be within the metricsRegionServer != null check. Maybe move this and the above line up outside
There was a problem hiding this comment.
Also you missed adding this to append
bbeaudreault
reviewed
Feb 2, 2024
| @@ -3041,6 +3048,7 @@ private CheckAndMutateResult checkAndMutate(HRegion region, OperationQuota quota | |||
| long after = EnvironmentEdgeManager.currentTime(); | |||
| long blockBytesScanned = | |||
| context != null ? context.getBlockBytesScanned() - blockBytesScannedBefore : 0; | |||
| quota.addBlockBytesScanned(blockBytesScanned); | |||
bbeaudreault
reviewed
Feb 2, 2024
| @@ -2841,6 +2845,9 @@ public MultiResponse multi(final RpcController rpcc, final MultiRequest request) | |||
| spaceQuotaEnforcement); | |||
| } | |||
| } finally { | |||
| if (context != null) { | |||
There was a problem hiding this comment.
I dont think this works out so well, because of how some of the methods are abstracted. For example, increment can be called from the single mutate method and also from doNonAtomicRegionMutation (which is called from multi). So for increments, we'd double account them.
I might recommend only adding calls to addBlockBytesScanned wherever there are existing calls to getBlockBytesScanned() for metrics. As I mentioned in another comment, you'll want to move the getBlockBytesScanned() outside the if (metrics != null) checks, but otherwise those are probably the best places to add the quota calls if possible.
There was a problem hiding this comment.
Hmm actually maybe it's better to keep this here and to instead handle avoiding double counting append/increment, i.e. by passing in a isMulti boolean or something
bbeaudreault
reviewed
Feb 2, 2024
| readConsumed = estimateConsume(OperationType.GET, numReads, 100); | ||
| readConsumed += estimateConsume(OperationType.SCAN, numScans, 1000); | ||
| if (useBlockBytesScanned) { | ||
| writeConsumed = estimateConsume(OperationType.MUTATE, numWrites, 100); |
There was a problem hiding this comment.
this is a nit, but can you move the writeConsumed out of the if blocks so we don't duplicate?
|
🎊 +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. |
rmdmattingly
force-pushed
the
HBASE-27687
branch
from
9d07c44 to
d167196
Compare
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Feb 6, 2024
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
That test failure looks unrelated |
bbeaudreault
pushed a commit
that referenced
this pull request
Feb 6, 2024
…esponse size (#5654) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault
pushed a commit
that referenced
this pull request
Feb 6, 2024
…esponse size (#5654) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault
pushed a commit
that referenced
this pull request
Feb 6, 2024
…esponse size (#5654) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault
pushed a commit
to HubSpot/hbase
that referenced
this pull request
Feb 6, 2024
…nned rather than response size (apache#5654) 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.
See https://issues.apache.org/jira/browse/HBASE-27687
We've found that result size is not a great indication of a request's workload for the server. For example, as is, one may set a user to have a 100mb/machine throttle and then find that the given user is able to generate far more than 100mb of IO against any given machine if their result sizes are significantly smaller than the IO required to facilitate the request.
This PR introduces
hbase.quota.use.block.bytes.scanned, a configurable boolean (defaulted to false) which, if true, will cause quotas to consume read availability via block bytes scanned rather than result size. Extending the aforementioned example, this should help to ensure that a user throttled to 100mb of IO/machine will not be able to exceed 100mb of IO/machine.I've deployed this to a test cluster and confirmed the intended behavior. For example, below is a chart of the MB/sec IO footprint across 4 RegionServers serving a heavily filtered scan workload coming from a user with a 100MB/sec throttle configured. As you can see, in the beginning, each host is far exceeding 100MB/sec of IO (because it is measuring throttle usage via result sizes, which are small post-filtering). In this timeframe we roll each host to use a build of HBase which includes this PR's changeset (and a configuration of
hbase.quota.use.block.bytes.scannedto true), and consequently we see the erroneously large IO workloads all throttle back to basically exactly 100MB/sec:@bbeaudreault @hgromer @eab148 @bozzkar