HBASE-29141 Calculate default maxQueueLength call queues correctly#7490
Merged
apurtell merged 5 commits intoapache:masterfrom Jan 26, 2026
Merged
HBASE-29141 Calculate default maxQueueLength call queues correctly#7490apurtell merged 5 commits intoapache:masterfrom
apurtell merged 5 commits intoapache:masterfrom
Conversation
Umeshkumar9414
changed the title
Umeshkumar9414
force-pushed
the
HBASE-29141
branch
from
4b58ef5 to
86285d4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@Apache9 , @apurtell , @virajjasani need help in review. |
mnpoonia
approved these changes
Nov 30, 2025
Contributor
mnpoonia
left a comment
mnpoonia
left a comment
There was a problem hiding this comment.
@Umeshkumar9414 This is surely a change in good direction.
d-c-manning
reviewed
Dec 1, 2025
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BalancedQueueRpcExecutor.java
Outdated
Show resolved
Hide resolved
| currentQueueLimit = (int) queueInitArgs[0]; | ||
| queueInitArgs[0] = Math.max((int) queueInitArgs[0], DEFAULT_CALL_QUEUE_SIZE_HARD_LIMIT); | ||
| queueInitArgs[0] = Math.min((int) queueInitArgs[0], DEFAULT_CALL_QUEUE_SIZE_HARD_LIMIT); | ||
| // queue should neven be initialised with 0 or less length |
Contributor
There was a problem hiding this comment.
nit typo
Suggested change
| // queue should neven be initialised with 0 or less length | |
| // queue should never be initialised with 0 or less length |
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcExecutor.java
Outdated
Show resolved
Hide resolved
| PriorityFunction qosFunction = mock(PriorityFunction.class); | ||
| RWQueueRpcExecutor executor = | ||
| new RWQueueRpcExecutor(testName.getMethodName(), 100, 100, qosFunction, conf, null); | ||
| RWQueueRpcExecutor executor = new RWQueueRpcExecutor(testName.getMethodName(), 100, |
Contributor
There was a problem hiding this comment.
Could we have a test that validates the queue length limits? Could we validate RpcExecutor#currentQueueLimit directly? Or some other validation that tries to fill the queue, but that may require more work and complexity.
Umeshkumar9414
force-pushed
the
HBASE-29141
branch
from
86285d4 to
2835b51
Compare
Umeshkumar9414
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
I tried both the tests (TestAcidGuaranteesWithBasicPolicy and TestAcidGuaranteesWithEagerPolicy) on my local and both are working fine. |
This comment has been minimized.
This comment has been minimized.
Umeshkumar9414
force-pushed
the
HBASE-29141
branch
from
82b8ce4 to
a826f1e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d-c-manning
approved these changes
Jan 13, 2026
| .collect(Collectors.groupingBy(Pair::getFirst, Collectors.summingLong(Pair::getSecond))); | ||
| } | ||
|
|
||
| // IMPORTANT: Call this method only ONCE per executor instance. |
Contributor
There was a problem hiding this comment.
I wonder if we could just enforce this in the code? Only allow it to be called once, and if it is called a second time then throw an "already initialized" error?
| handlerCount * RpcServer.DEFAULT_MAX_CALLQUEUE_LENGTH_PER_HANDLER); | ||
| int maxPriorityQueueLength = conf.getInt(RpcScheduler.IPC_SERVER_PRIORITY_MAX_CALLQUEUE_LENGTH, | ||
| priorityHandlerCount * RpcServer.DEFAULT_MAX_CALLQUEUE_LENGTH_PER_HANDLER); | ||
| int maxQueueLength = conf.getInt(RpcScheduler.IPC_SERVER_MAX_CALLQUEUE_LENGTH, -1); |
Contributor
There was a problem hiding this comment.
can we make the -1 a constant too? something like MAX_CALLQUEUE_LENGTH_UNDEFINED?
Suggested change
| int maxQueueLength = conf.getInt(RpcScheduler.IPC_SERVER_MAX_CALLQUEUE_LENGTH, -1); | |
| int maxQueueLength = conf.getInt(RpcScheduler.IPC_SERVER_MAX_CALLQUEUE_LENGTH, MAX_CALLQUEUE_LENGTH_UNDEFINED); |
| private static final QueueBalancer ONE_QUEUE = val -> 0; | ||
|
|
||
| public static QueueBalancer getBalancer(final String executorName, final Configuration conf, | ||
| protected static QueueBalancer getBalancer(final String executorName, final Configuration conf, |
Contributor
There was a problem hiding this comment.
this does seem like a useful visibility change, but must it be included here? Changing a public method of a "public" class (LimitedPrivate for COPROC, PHOENIX) is a breaking change. That being said, it's hard to imagine how anyone is using it outside of hbase... and the return class is not LimitedPrivate for COPROC... and I don't see it used in Phoenix... so 🤷♂️
Contributor
Author
There was a problem hiding this comment.
This method creates the balancer for RPC queues, so I don't think it should be visible outside of the protected visibility.
Umeshkumar9414
force-pushed
the
HBASE-29141
branch
from
a826f1e to
394ad47
Compare
This was referenced Jan 13, 2026
This comment has been minimized.
This comment has been minimized.
d-c-manning
approved these changes
Jan 13, 2026
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Both test - TestAcidGuaranteesWithBasicPolicy,TestAcidGuaranteesWithEagerPolicy, are working in local. Seems like flacky or ongoing issue. |
Umeshkumar9414
force-pushed
the
HBASE-29141
branch
from
394ad47 to
5d18bb6
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
apurtell
approved these changes
Jan 21, 2026
Contributor
apurtell
left a comment
apurtell
left a comment
There was a problem hiding this comment.
+1, will merge soon unless objection
Contributor
|
In order to get clean cherry picks back to branch-2 based code, #7620 should be merged first. |
Contributor
Author
asf-gitbox-commits
pushed a commit
that referenced
this pull request
Jan 26, 2026
…7490) Co-authored-by: ukumawat <ukumawat@salesforce.com> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asf-gitbox-commits
pushed a commit
that referenced
this pull request
Jan 26, 2026
…7490) Co-authored-by: ukumawat <ukumawat@salesforce.com> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asf-gitbox-commits
pushed a commit
that referenced
this pull request
Jan 26, 2026
…7490) Co-authored-by: ukumawat <ukumawat@salesforce.com> Signed-off-by: Andrew Purtell <apurtell@apache.org>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Issues solved -