Skip to content

Conversation

rmdmattingly
Copy link
Contributor

Right now any modifications to Quotas' underlying TimeBasedLimiters' configurations require a rolling restart to take effect. In an ideal world, it would be easier to make / rollback changes to something so impactful.

As part of https://issues.apache.org/jira/browse/HBASE-29351, I've been working on a smart rate limiter at my day job, and I'd like to contribute it to the open-source project. It will be much easier to work with a more sophisticated rate limiter that can be reconfigured more trivially

@rmdmattingly rmdmattingly requested a review from Copilot October 14, 2025 15:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enables dynamic configuration refresh for TimeBasedLimiters used in HBase quota management. Currently, any changes to quota limiter configurations require a rolling restart to take effect, which is operationally disruptive.

  • Modified TimeBasedLimiter and related quota classes to accept Configuration parameters for dynamic rate limiter selection
  • Updated quota fetching methods to pass Configuration objects through the call chain
  • Removed static Configuration instance from TimeBasedLimiter to enable configuration-aware rate limiter creation

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
TimeBasedLimiter.java Removed static configuration and modified constructor/factory method to accept Configuration parameter
QuotaLimiterFactory.java Updated factory method to accept Configuration parameter
QuotaState.java Modified setQuotas method to pass Configuration to quota limiter factory
UserQuotaState.java Updated setQuotas methods to accept and pass Configuration parameter
QuotaUtil.java Modified quota fetching methods to accept Configuration and pass it through quota creation
QuotaCache.java Updated to pass Configuration from services to quota fetching methods
TestQuotaState.java Added Configuration parameter to test methods
TestQuotaCache2.java Added Configuration parameter to test methods
TestDefaultOperationQuota.java Added Configuration parameter to test methods

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

public static Map<String, UserQuotaState> fetchUserQuotas(final Connection connection,
Map<TableName, Double> tableMachineQuotaFactors, double factor) throws IOException {
public static Map<String, UserQuotaState> fetchUserQuotas(final Configuration conf,
final Connection connection, Map<TableName, Double> tableMachineQuotaFactors, double factor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you retrieve the Configuration instance from the Connection instance? Or do you expect that something is passing a conf object that they've modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether it's guaranteed that the connection passed here will always have a dynamically refreshed configuration. Feel like the intention is more clear with this as an explicit arg, but I agree the redundant arg is a little painful...

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 50s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
_ master Compile Tests _
+1 💚 mvninstall 4m 49s master passed
+1 💚 compile 5m 7s master passed
+1 💚 checkstyle 1m 8s master passed
+1 💚 spotbugs 2m 37s master passed
+1 💚 spotless 1m 18s branch has no errors when running spotless:check.
_ Patch Compile Tests _
+1 💚 mvninstall 4m 24s the patch passed
+1 💚 compile 4m 53s the patch passed
+1 💚 javac 4m 53s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 0m 59s the patch passed
+1 💚 spotbugs 2m 40s the patch passed
+1 💚 hadoopcheck 16m 48s Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
+1 💚 spotless 1m 10s patch has no errors when running spotless:check.
_ Other Tests _
+1 💚 asflicense 0m 15s The patch does not generate ASF License warnings.
57m 1s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7387/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #7387
Optional Tests dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname Linux d369e3e7ce8b 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 91e0011
Default Java Eclipse Adoptium-17.0.11+9
Max. process+thread count 84 (vs. ulimit of 30000)
modules C: hbase-server U: hbase-server
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7387/4/console
versions git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 36s Docker mode activated.
-0 ⚠️ yetus 0m 3s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚 mvninstall 4m 37s master passed
+1 💚 compile 1m 24s master passed
+1 💚 javadoc 0m 50s master passed
+1 💚 shadedjars 7m 23s branch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+1 💚 mvninstall 3m 38s the patch passed
+1 💚 compile 1m 6s the patch passed
+1 💚 javac 1m 6s the patch passed
+1 💚 javadoc 0m 30s the patch passed
+1 💚 shadedjars 6m 44s patch has no errors when building our shaded downstream artifacts.
_ Other Tests _
-1 ❌ unit 252m 14s /patch-unit-hbase-server.txt hbase-server in the patch failed.
284m 15s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7387/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #7387
Optional Tests javac javadoc unit compile shadedjars
uname Linux 89d66b7de0ee 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 91e0011
Default Java Eclipse Adoptium-17.0.11+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7387/4/testReport/
Max. process+thread count 4383 (vs. ulimit of 30000)
modules C: hbase-server U: hbase-server
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7387/4/console
versions git=2.34.1 maven=3.9.8
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@rmdmattingly
Copy link
Contributor Author

Test failures are now unrelated:

WARNING: System::setSecurityManager will be removed in a future release
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 196.2 s -- in org.apache.hadoop.hbase.client.TestMobSnapshotCloneIndependence
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR] org.apache.hadoop.hbase.regionserver.TestTags.testFlushAndCompactionwithCombinations
[ERROR]   Run 1: TestTags.testFlushAndCompactionwithCombinations:419 expected:<1> but was:<0>
[ERROR]   Run 2: TestTags.testFlushAndCompactionwithCombinations:444 expected:<1> but was:<0>
[ERROR]   Run 3: TestTags.testFlushAndCompactionwithCombinations:419 expected:<1> but was:<0>
[INFO] 
[ERROR] Errors: 
[ERROR] org.apache.hadoop.hbase.master.assignment.TestRollbackSCP.testFailAndRollback
[ERROR]   Run 1: TestRollbackSCP.testFailAndRollback:180 » IllegalArgument scheduler queue not empty: MasterProcedureScheduler[running=true,tableMap=TableQueue[key=test,lockStatus=LockAndQueue[exclusiveLock=false,sharedLockCount=0],size=1,namespaceLockStatus=LockAndQueue[exclusiveLock=false,sharedLockCount=0]],tableWaitingMap={},peerMap=<null>,metaMap=<null>,globalMap=<null>]
[ERROR]   Run 2: TestRollbackSCP.testFailAndRollback:177 Waiting timed out after [30,000] msec
[ERROR]   Run 3: TestRollbackSCP.testFailAndRollback:177 Waiting timed out after [30,000] msec
[INFO] 
[WARNING] Flakes: 
[WARNING] org.apache.hadoop.hbase.ipc.TestBlockingIPC.testBadPreambleHeader[0: rpcServerImpl=class org.apache.hadoop.hbase.ipc.SimpleRpcServer]
[ERROR]   Run 1: TestBlockingIPC>AbstractTestIPC.testBadPreambleHeader:642 Can not get expected BadAuthException
[INFO]   Run 2: PASS
[INFO] 
[WARNING] org.apache.hadoop.hbase.rsgroup.TestRSGroupsAdmin1.testNonExistentTableMove
[ERROR]   Run 1: TestRSGroupsAdmin1.testNonExistentTableMove:396 Table GrouptestNonExistentTableMove shouldn't have been successfully moved.
[INFO]   Run 2: PASS
[INFO] 
[INFO] 
[ERROR] Tests run: 6583, Failures: 1, Errors: 1, Skipped: 67, Flakes: 2

@rmdmattingly rmdmattingly merged commit dfca61b into apache:master Oct 16, 2025
1 check failed
@rmdmattingly rmdmattingly deleted the HBASE-29663 branch October 16, 2025 13:12
rmdmattingly added a commit that referenced this pull request Oct 16, 2025
…fresh (#7387)

Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
Signed-off-by: Charles Connell <cconnell@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
rmdmattingly added a commit that referenced this pull request Oct 16, 2025
…fresh (#7387)

Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
Signed-off-by: Charles Connell <cconnell@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
rmdmattingly added a commit that referenced this pull request Oct 17, 2025
…fresh (#7387) (#7392)

Signed-off-by: Charles Connell <cconnell@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
rmdmattingly added a commit that referenced this pull request Oct 17, 2025
…fresh (#7387) (#7393)

Signed-off-by: Charles Connell <cconnell@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
rmdmattingly added a commit that referenced this pull request Oct 17, 2025
…fresh (#7387) (#7393)

Signed-off-by: Charles Connell <cconnell@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
rmdmattingly added a commit that referenced this pull request Oct 17, 2025
…fresh (#7387) (#7393) (#7395)

Signed-off-by: Charles Connell <cconnell@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants