-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-26703 Allow configuration of IPC queue balancer #4063
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
ea1719f
to
a6cbfaf
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
a6cbfaf
to
8262461
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
8262461
to
aa95b4a
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
03c3650
to
9853975
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
One minor issue, otherwise lgtm.
return ThreadLocalRandom.current().nextInt(queueSize); | ||
} | ||
|
||
@RestrictedApi(explanation = "Should only be called in tests", link = "", |
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.
Can we not do this? I don't want to tie our code to Google annotation types given their penchant for breaking changes. A comment would suffice.
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.
Sounds good! Was just trying to follow convention, but happy to remove. Thank you for reviewing.
9853975
to
04d122a
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The test failure here looks like a flakey test timeout. I can try a force push to kick it off again, but probably safe to merge? |
It seems fine. Merging, picking to branch-2 and branch-2.5. |
Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcExecutor.java
Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcExecutor.java
Since RandomQueueBalancer is now just the default option of a new config, I thought it might make sense to split it and QueueBalancer out into top-level classes. Let me know if anyone disagrees with the IA/IS chosen here.
This change is covered by tests in TestSimpleRpcScheduler, but I also added an explicit test for the slicing of lists done in RWQueueRpcExecutor.
I want to pass in the actual queues to the balancer factory because some balancers may want to introspect the queues in determining how to balance. For example, a load-based balancer might want to check the size() to determine the least-loaded queue.