wchevreuil commented on a change in pull request #764: HBASE-23223 Support the
offsetLock of bucketCache to use strong ref
URL: https://github.com/apache/hbase/pull/764#discussion_r339990944
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
##########
@@ -42,93 +21,7 @@
* For write lock, use lock.writeLock()
*/
@InterfaceAudience.Private
-public class IdReadWriteLock<T> {
- // The number of lock we want to easily support. It's not a maximum.
- private static final int NB_CONCURRENT_LOCKS = 1000;
- /**
- * The pool to get entry from, entries are mapped by {@link Reference} and
will be automatically
- * garbage-collected by JVM
- */
- private final ObjectPool<T, ReentrantReadWriteLock> lockPool;
- private final ReferenceType refType;
-
- public IdReadWriteLock() {
- this(ReferenceType.WEAK);
- }
-
- /**
- * Constructor of IdReadWriteLock
- * @param referenceType type of the reference used in lock pool, {@link
ReferenceType#WEAK} by
- * default. Use {@link ReferenceType#SOFT} if the key set is
limited and the locks will
- * be reused with a high frequency
- */
- public IdReadWriteLock(ReferenceType referenceType) {
- this.refType = referenceType;
- switch (referenceType) {
Review comment:
Ok, maybe _ObjectPool_ can be made generic enough to hold Object instances,
then _Reference_ instances would be moved to the specific _Soft_ and _Weak_
pool implementations? That way we could have also a _Strong_ pool one. My point
here is to try keep _IdReadWriteLock_ as the single point of access for
whatever type of locks might be needed, not only for code reuse, but as a more
intuitive API.
In the case we still find too difficult to follow the above approach, then
we should leave _waitForWaiters_ method implementation on the parent
class/interface, instead of duplicating it in the two subclasses. Also, can we
go with a more generic name for _IdReadWriteLockSoftOrWeakRef_? What if in the
future someone finds useful add a phantom reference or any other type of lock
references, that would then require yet another class rename.
All the above said, can you share some thoughts on the review main comments?
I'm still concerned on the overall impact in the long run. Would be good to
have some more details on your scenario.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
