d-c-manning commented on code in PR #7490:
URL: https://github.com/apache/hbase/pull/7490#discussion_r2575758014
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestRWQueueRpcExecutor.java:
##########
@@ -54,15 +55,16 @@ public class TestRWQueueRpcExecutor {
public void setUp() {
conf = HBaseConfiguration.create();
conf.setFloat(CALL_QUEUE_HANDLER_FACTOR_CONF_KEY, 1.0f);
+ conf.setInt(IPC_SERVER_MAX_CALLQUEUE_LENGTH, 100);
conf.setFloat(CALL_QUEUE_SCAN_SHARE_CONF_KEY, 0.5f);
conf.setFloat(CALL_QUEUE_READ_SHARE_CONF_KEY, 0.5f);
}
@Test
public void itProvidesCorrectQueuesToBalancers() throws InterruptedException
{
PriorityFunction qosFunction = mock(PriorityFunction.class);
- RWQueueRpcExecutor executor =
- new RWQueueRpcExecutor(testName.getMethodName(), 100, 100, qosFunction,
conf, null);
+ RWQueueRpcExecutor executor = new
RWQueueRpcExecutor(testName.getMethodName(), 100,
Review 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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BalancedQueueRpcExecutor.java:
##########
@@ -37,16 +37,16 @@ public class BalancedQueueRpcExecutor extends RpcExecutor {
private final QueueBalancer balancer;
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,
Review 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?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcExecutor.java:
##########
@@ -225,7 +229,9 @@ public Map<String, Long> getCallQueueSizeSummary() {
protected void initializeQueues(final int numQueues) {
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);
Review 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](https://issues.apache.org/jira/browse/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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcExecutor.java:
##########
@@ -225,7 +229,9 @@ public Map<String, Long> getCallQueueSizeSummary() {
protected void initializeQueues(final int numQueues) {
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);
+ // queue should neven be initialised with 0 or less length
Review Comment:
nit typo
```suggestion
// queue should never be initialised with 0 or less length
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
