Skip to content

Conversation

@Umeshkumar9414
Copy link
Contributor

@Umeshkumar9414 Umeshkumar9414 commented Nov 28, 2025

Issues solved -

  • While calculating default maxQueueLength, all handlers are considered instead of handler of one queue.
  • Soft limit was getting overridden becuase calling initializeQueues multiple times.

@Umeshkumar9414 Umeshkumar9414 changed the title @HBASE-29141 Resolution for high default maxQueueLength for call queues HBASE-29141 Resolution for high default maxQueueLength for call queues Nov 28, 2025
@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Umeshkumar9414
Copy link
Contributor Author

@Apache9 , @apurtell , @virajjasani need help in review.

Copy link
Contributor

@mnpoonia mnpoonia left a comment

Choose a reason for hiding this comment

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

@Umeshkumar9414 This is surely a change in good direction.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit typo

Suggested change
// queue should neven be initialised with 0 or less length
// queue should never be initialised with 0 or less length

PriorityFunction qosFunction = mock(PriorityFunction.class);
RWQueueRpcExecutor executor =
new RWQueueRpcExecutor(testName.getMethodName(), 100, 100, qosFunction, conf, null);
RWQueueRpcExecutor executor = new RWQueueRpcExecutor(testName.getMethodName(), 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test

@Umeshkumar9414 Umeshkumar9414 changed the title HBASE-29141 Resolution for high default maxQueueLength for call queues HBASE-29141 Calculate default maxQueueLength call queues correctly Jan 8, 2026
@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Umeshkumar9414
Copy link
Contributor Author

I tried both the tests (TestAcidGuaranteesWithBasicPolicy and TestAcidGuaranteesWithEagerPolicy) on my local and both are working fine.
@Apache9 , @d-c-manning , @apurtell can you please review this PR.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

.collect(Collectors.groupingBy(Pair::getFirst, Collectors.summingLong(Pair::getSecond)));
}

// IMPORTANT: Call this method only ONCE per executor instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method creates the balancer for RPC queues, so I don't think it should be visible outside of the protected visibility.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 16s 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 3m 1s master passed
+1 💚 compile 5m 8s master passed
+1 💚 checkstyle 1m 1s master passed
+1 💚 spotbugs 1m 42s master passed
+1 💚 spotless 0m 47s branch has no errors when running spotless:check.
_ Patch Compile Tests _
+1 💚 mvninstall 2m 55s the patch passed
+1 💚 compile 5m 7s the patch passed
+1 💚 javac 5m 7s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 1m 0s hbase-server: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)
+1 💚 spotbugs 1m 51s the patch passed
+1 💚 hadoopcheck 11m 32s Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
+1 💚 spotless 0m 46s patch has no errors when running spotless:check.
_ Other Tests _
+1 💚 asflicense 0m 10s The patch does not generate ASF License warnings.
42m 54s
Subsystem Report/Notes
Docker ClientAPI=1.52 ServerAPI=1.52 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7490/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #7490
Optional Tests dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname Linux 5901cb989c05 6.14.0-1018-aws #18~24.04.1-Ubuntu SMP Mon Nov 24 19:46:27 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 394ad47
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-7490/5/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 13s Docker mode activated.
-0 ⚠️ yetus 0m 4s 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 2m 51s master passed
+1 💚 compile 1m 0s master passed
+1 💚 javadoc 0m 28s master passed
+1 💚 shadedjars 6m 23s branch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+1 💚 mvninstall 2m 58s the patch passed
+1 💚 compile 1m 2s the patch passed
+1 💚 javac 1m 2s the patch passed
+1 💚 javadoc 0m 29s the patch passed
+1 💚 shadedjars 6m 19s patch has no errors when building our shaded downstream artifacts.
_ Other Tests _
-1 ❌ unit 248m 17s /patch-unit-hbase-server.txt hbase-server in the patch failed.
274m 35s
Subsystem Report/Notes
Docker ClientAPI=1.52 ServerAPI=1.52 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7490/5/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #7490
Optional Tests javac javadoc unit compile shadedjars
uname Linux fa24ffe25adf 6.14.0-1018-aws #18~24.04.1-Ubuntu SMP Mon Nov 24 19:46:27 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 394ad47
Default Java Eclipse Adoptium-17.0.11+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7490/5/testReport/
Max. process+thread count 4287 (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-7490/5/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.

@Umeshkumar9414
Copy link
Contributor Author

Both test - TestAcidGuaranteesWithBasicPolicy,TestAcidGuaranteesWithEagerPolicy, are working in local. Seems like flacky or ongoing issue.

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.

5 participants