-
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-27064 CME in TestRegionNormalizerWorkQueue #4468
Conversation
|
💔 -1 overall
This message was automatically generated. |
spotless:apply |
🎊 +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. |
Latest precommit is good. |
Rebased. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
takeLock.unlock(); | ||
putLock.unlock(); | ||
lock = new ReentrantReadWriteLock(); | ||
notEmpty = lock.writeLock().newCondition(); |
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.
Basically we are changing notEmpty from being readLock Condition to writeLock Condition, this seems good.
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.
ReentrantReadWriteLock does not support Conditions on the readLock. It would throw UnsupportedOperationException.
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
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.
This looks simpler and there's no reason for this code to be complex, so let's try it.
*/ | ||
@InterfaceAudience.Private | ||
class RegionNormalizerWorkQueue<E> { | ||
|
||
/** Underlying storage structure that gives us the Set behavior and FIFO retrieval policy. */ | ||
private LinkedHashSet<E> delegate; | ||
|
||
// the locking structure used here follows the example found in LinkedBlockingQueue. The |
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.
It's super concerning that there's an issue here because, per this comment, the implementation follows the locking implementation of LinkedBlockingQueue
.
Signed-off-by: Viraj Jasani <vjasani@apache.org>
This reverts commit a93ea9e.
…e) to 2.4 (apache#4794) (apache#4468) Co-authored-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit f24e7b5) Change-Id: Ie277a2e130456a45757c2ef9ffdaa2ee8cb52c26
My impression after a quick read of the code is the separate locks
putLock
andtakeLock
should be unified as a reentrant read-write lock.