-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-27355 Separate meta read requests from master and client #7261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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 separates meta read requests from master and client by introducing different QoS (Quality of Service) levels for internal vs client meta operations. The change allows internal meta read requests to be processed in separate queues from client requests to improve system performance and isolation.
Key changes:
- Introduces
INTERNAL_READ_QOSconstant for internal meta read operations - Updates
MetaRWQueueRpcExecutorto dispatch internal reads to scan queues and client reads to read queues - Sets priority for internal meta operations using the new QoS level
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| RSAnnotationReadingPriorityFunction.java | Adds public constant INTERNAL_READ_QOS and makes class public |
| MetaRWQueueRpcExecutor.java | Implements custom dispatch logic to separate internal vs client reads using QoS levels |
| RpcExecutor.java | Extracts call queue handler factor configuration to protected method |
| RWQueueRpcExecutor.java | Adds getter method for number of scan queues |
| MetaTableAccessor.java | Sets internal read priority on meta operations and fixes logging issues |
| TestSimpleRpcScheduler.java | Updates test mocks to include priority headers and fixes configuration keys |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // QOS for internal meta read requests | ||
| public static int INTERNAL_READ_QOS = 250; |
Copilot
AI
Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The INTERNAL_READ_QOS field should be declared as final since it represents a constant value that should not be modified after initialization.
| // QOS for internal meta read requests | |
| public static int INTERNAL_READ_QOS = 250; | |
| public static final int INTERNAL_READ_QOS = 250; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The failed unit tests looks unrelated. Add a new commit to run tests again. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Some unit tests failed again. This is a little weird, let me check the reason. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mnpoonia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much needed functionality.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Still working on finding out why this PR makes some tests flaky ..... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally we got a clean run. But let's make sure there is no other problems...
The default config for running UTs usually uses small values, for example, hbase.regionserver.metahandler.count is 3 instead of 20, will this cause problems with the new code?
@frostruan Thanks.
|
|
||
| @Override | ||
| protected float getCallQueueHandlerFactor(Configuration conf) { | ||
| return conf.getFloat(META_CALL_QUEUE_HANDLER_FACTOR_CONF_KEY, 0.5f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind explaining why here we use 0.5 instead of 0.1 for default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial thinking was that because we always cache meta table in memory, meta handlers would have lower latency than regular handlers. So, a larger handler factor would make meta handlers more efficient compared to regular handlers because it would reducce thread synchronization cost when consuming queues. I haven't tested this, it is just speculation.
Thanks for reminding duo @Apache9 , it's possible, let me check again. But I just can't figure it out, this PR only changes the queue to which a request should be dispatched. Why does it break TestTableSnapshotScannerWithSFT and TestTableSnapshotScanner for 3 times ? These two don't seem to be related, and I can't reproduce it locally. |
If meta handler is not correctly set, any tests can hang since the request to meta may be delayed or blocked, I guess. Or maybe it is just some machine issues... |
|
|
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Co-authored-by: huiruan <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Aman Poonia <[email protected]> (cherry picked from commit 89416ce)
No description provided.