Uh oh!
There was an error while loading. Please reload this page.
HBASE-29281 Atomic request throttles are missing QuotaSettingsFactory support - #6953
Merged
Conversation
Comment on lines
+661
to
+663
| for (ThrottleType throttleType : ThrottleType.values()) { | ||
| canSetAndGetUserThrottle(throttleType); | ||
| } |
ContributorAuthor
There was a problem hiding this comment.
By validating this for every throttle type, we'll prevent new throttle types from missing this key usability piece down the road
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the missing support for atomic throttle settings in the QuotaSettingsFactory and adds tests to validate that all throttle types—including the new atomic ones—are handled correctly.
- Updated QuotaSettingsFactory.java to process atomic read size, write size, and request number throttles.
- Added a new test method in TestQuotaThrottle.java to iterate over all throttle types and verify that the quota settings are applied and subsequently removed.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaThrottle.java | Added tests to verify correct setting and removal of all throttle types. |
| hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/QuotaSettingsFactory.java | Extended throttle settings construction to include atomic throttle support. |
Comments suppressed due to low confidence (1)
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaThrottle.java:690
- Consider asserting that the quota for the given throttleType has been successfully removed after unthrottling, to enhance test coverage and validate the unthrottling behavior.
admin.setQuota(QuotaSettingsFactory.unthrottleUserByThrottleType(userName, throttleType));
Comment on lines
170
to
+178
| } | ||
| if (throttle.hasAtomicReadSize()) { | ||
| settings.add(ThrottleSettings.fromTimedQuota(userName, tableName, namespace, regionServer, | ||
| ThrottleType.ATOMIC_READ_SIZE, throttle.getAtomicReadSize())); | ||
| } | ||
| if (throttle.hasAtomicWriteSize()) { | ||
| settings.add(ThrottleSettings.fromTimedQuota(userName, tableName, namespace, regionServer, | ||
| ThrottleType.ATOMIC_WRITE_SIZE, throttle.getAtomicWriteSize())); | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment here (and similarly for atomic write and request numbers) to explain the purpose of handling these atomic throttle settings for improved maintainability.
Suggested change
| } | |
| if (throttle.hasAtomicReadSize()) { | |
| settings.add(ThrottleSettings.fromTimedQuota(userName, tableName, namespace, regionServer, | |
| ThrottleType.ATOMIC_READ_SIZE, throttle.getAtomicReadSize())); | |
| } | |
| if (throttle.hasAtomicWriteSize()) { | |
| settings.add(ThrottleSettings.fromTimedQuota(userName, tableName, namespace, regionServer, | |
| ThrottleType.ATOMIC_WRITE_SIZE, throttle.getAtomicWriteSize())); | |
| } | |
| } | |
| // Handle atomic read size throttling: This enforces limits on the size of read operations | |
| // at an atomic level, ensuring precise control over resource usage. | |
| if (throttle.hasAtomicReadSize()) { | |
| settings.add(ThrottleSettings.fromTimedQuota(userName, tableName, namespace, regionServer, | |
| ThrottleType.ATOMIC_READ_SIZE, throttle.getAtomicReadSize())); | |
| } | |
| // Handle atomic write size throttling: This enforces limits on the size of write operations | |
| // at an atomic level, ensuring precise control over resource usage. | |
| if (throttle.hasAtomicWriteSize()) { | |
| settings.add(ThrottleSettings.fromTimedQuota(userName, tableName, namespace, regionServer, | |
| ThrottleType.ATOMIC_WRITE_SIZE, throttle.getAtomicWriteSize())); | |
| } | |
| // Handle atomic request number throttling: This enforces limits on the number of requests | |
| // at an atomic level, ensuring precise control over resource usage. |
ContributorAuthor
There was a problem hiding this comment.
I'm against superfluous comments that just reiterate straightforward code. This is already too verbose
Apache-HBase
commented
Apr 30, 2025
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 30, 2025
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
May 1, 2025
rmdmattingly added a commit
that referenced
this pull request
May 1, 2025
… support (#6953) Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
rmdmattingly added a commit
that referenced
this pull request
May 1, 2025
… support (#6953) Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
rmdmattingly added a commit
that referenced
this pull request
May 1, 2025
… support (#6953) Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
mokai87 pushed a commit
to mokai87/hbase
that referenced
this pull request
Aug 7, 2025
… support (apache#6953) (apache#6957) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
This file contains hidden or 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
Sign up for freeto 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.
While rolling out atomic throttles at my day job, I realized that we're unable to fetch our atomic throttle settings. This is because, in https://issues.apache.org/jira/browse/HBASE-29229, I forgot to wire up atomic throttles in the QuotaSettingsFactory.
I've also added a test that will make it much harder to make this same mistake down the road