HBASE-29141 Resolution for high default maxQueueLength for call queues #7490
Conversation
Umeshkumar9414
changed the title
Umeshkumar9414
force-pushed
the
HBASE-29141
branch
from
4b58ef5 to
86285d4
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 , @apurtell , @virajjasani need help in review. |
mnpoonia
left a comment
mnpoonia
left a comment
There was a problem hiding this comment.
@Umeshkumar9414 This is surely a change in good direction.
|
|
||
| public BalancedQueueRpcExecutor(final String name, final int handlerCount, | ||
| final int maxQueueLength, final PriorityFunction priority, final Configuration conf, | ||
| final String maxQueueLengthConfKey, final PriorityFunction priority, final Configuration conf, |
There was a problem hiding this comment.
can we make the logic change without changing the API signature which is used by HBaseInterfaceAudience.COPROC and PHOENIX?
What if we pass -1 for maxQueueLength, and if it is found to be -1, then determine it using the calculation you have added in this PR?
There was a problem hiding this comment.
Yes, I agree changing API signature might not work here.
But somehow we have to pass the config value or config key from outside only. As this same constructor is being used for meta priority handler and normal handler.
I think adding an new constructor might be a way forword
| 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 |
There was a problem hiding this comment.
nit typo
| // queue should neven be initialised with 0 or less length | |
| // queue should never be initialised with 0 or less length |
| if (queueInitArgs.length > 0) { | ||
| 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); |
There was a problem hiding this comment.
This doesn't seem like a safe change, considering people may be choosing larger queue sizes than 250 today. But this change would enforce no queue size larger than 250.
The pre-existing logic confuses me anyway. It talks about hard limits and soft limits, and suggests that we will initialize queues with hard limits (for initial capacity concerns?) but then enforce soft limits. It was added with HBASE-15306 which talks about dynamic reconfiguration of queues.
But it seems that with RWQueueRpcExecutor, when we call initializeQueues three times, we would be setting the "soft" (current) limit to be this "hard" 250 limit, even if it was greater than the provided/calculated maxQueueLength. Probably the code does not expect the case of calling initializeQueues multiple times.
So perhaps the logic does need to change here, but I don't think we can simply change to Math.min either.
There was a problem hiding this comment.
Than @d-c-manning it helped me understand hard limit and soft limit in a better way.
| PriorityFunction qosFunction = mock(PriorityFunction.class); | ||
| RWQueueRpcExecutor executor = | ||
| new RWQueueRpcExecutor(testName.getMethodName(), 100, 100, qosFunction, conf, null); | ||
| RWQueueRpcExecutor executor = new RWQueueRpcExecutor(testName.getMethodName(), 100, |
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.
4 participants
No description provided.