HBASE-27458 Use ReadWriteLock for region scanner readpoint map #4859
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Nov 2, 2022
| region.scannerReadPointsLock.readLock().lock(); | ||
| try { | ||
| this.readPt = calculateReadPoint(isolationLevel, mvccReadPoint, nonceGroup, nonce); | ||
| scannerReadPoints.put(this, this.readPt); |
There was a problem hiding this comment.
Here we also modified the scannerReadPoints? Why just use read lock here?
There was a problem hiding this comment.
Thanks for the review Duo
The scannerReadPoints is ConcurrentHashMap, which is thread-safe. So we can modified it concurrently. Shared lock is enough. But when getSmallestReadPoint, we need to ensure that scannerReadPoints can not be modified, we need exclusive lock.
So when modify it, we need to acquire shared lock and when read it, we need to acquire exclusive lock.
There was a problem hiding this comment.
Then I think what we want to protect here is not scannerReadPoints itself, but a special operation? So I prefer we change the name of the lock, maybe call it smallestReadPointsCalcLock?
There was a problem hiding this comment.
ok, let me rename it and fix the UT problems, Thanks Duo.
There was a problem hiding this comment.
And why do we need to introduce a new flag here?
There was a problem hiding this comment.
When we do delta operation like increment/append, we will need to get the smallest read point.
If we both have heavy delta operation load and heavy read load, the write lock may reduce the write performance. so let's introduce a flag to make this decided by the user.
There was a problem hiding this comment.
There was a problem hiding this comment.
Then I suggest we implement a class called ReadPointCalcuationLock, and introduce two types of lock, one is lockForCalculation, the other is lockForRecording. And we put the flag in this class, when it is enabled, we use a read write lock, and use write lock for calculation, and use read lock for recording. when it is disabled, we just use a lock.
I think this could make the code cleaner. WDYT?
Thanks.
There was a problem hiding this comment.
I agree. let me address it.
Thanks for your suggestion. Duo.
added 3 commits
November 2, 2022 22:49
Signed-off-by: huiruan <[email protected]>
Signed-off-by: huiruan <[email protected]>
Signed-off-by: huiruan <[email protected]>
frostruan
force-pushed
the
HBASE-27458
branch
from
83c7983 to
80fba10
Compare
|
Hi Duo @Apache9 I added a new commit. Would mind taking a look in your free time and see is this what you expect ? |
Apache9
reviewed
Nov 2, 2022
|
|
||
| ReadPointCalculationLock(Configuration conf) { | ||
| this.useReadWriteLockForReadPoints = | ||
| conf.getBoolean("hbase.readpoints.read.write.lock.enable", false); |
There was a problem hiding this comment.
Better name it hbase.region.xxx, as the lock is per region.
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| /** | ||
| * Lock to manage concurrency between RegionScanner and getSmallestReadPoint. |
There was a problem hiding this comment.
Please add more comments here to explain why we have this class and why we have a flag to control whether to use read write lock or not.
|
Thanks for reviewing Duo.Let's wait for the unit test results. |
|
🎊 +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. |
Apache9
approved these changes
Nov 3, 2022
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
sunhelly
pushed a commit
to sunhelly/hbase
that referenced
this pull request
Mar 2, 2023
…e#4859) Co-authored-by: huiruan <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
sunhelly
added a commit
that referenced
this pull request
Mar 2, 2023
#5073) Signed-off-by: Duo Zhang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.