-
Notifications
You must be signed in to change notification settings - Fork 9.1k
YARN-11656 RMStateStore event queue blocked #6569
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
💔 -1 overall
This message was automatically generated. |
- fix failing test - some test using RMStateStore without start it
- fix javadoc
* @return the semaphore | ||
*/ | ||
default String getLockKey() { | ||
return null; |
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.
Is returning null expected here?
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.
Hi @slfan1989 !
Thanks for the review.
Yes, that is expected.
If we dont specify lockKey for an event we should return with null, so these events will be executed in sequential not parallel. The method is used in the MultiDispatcherLocks.
- fix metric
💔 -1 overall
This message was automatically generated. |
- fix test - remove locks - add hash based dispatching
- some style fix
- fix thread name - sort log records in monitor
- fix out of array exception
💔 -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. |
- fix tests
💔 -1 overall
This message was automatically generated. |
- fix style
🎊 +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.
Thanks @K0K0V0K, I think it is a very nice patch! I only added some minor comments
String lockKey = event.getLockKey(); | ||
// abs of Integer.MIN_VALUE is Integer.MIN_VALUE | ||
int threadIndex = lockKey == null || lockKey.hashCode() == Integer.MIN_VALUE ? | ||
0 : Math.abs(lockKey.hashCode() % threads.length); |
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.
Based on our discussion, I think a comment or description probably would be useful to make the goal of this computation more clear
} | ||
|
||
/** | ||
* Maximus size of the event queue of the executor threads. |
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.
Just a typo, if you touch the code again anyways :)
💔 -1 overall
This message was automatically generated. |
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
Description of PR
Created a MultiDispatcher class which implements the Dispatcher interface.
The dispatcher replace the AsyncDispatcher in the RMStateStore.
The Dispatcher creates a separate metric object called Event metrics for "rm-state-store" where we can see
How was this patch tested?