HDDS-2244. Use new ReadWrite lock in OzoneManager. #1589
Conversation
bharatviswa504
force-pushed
the
HDDS-2244
branch
from
b372e7e
to
5893eb4
Compare
bharatviswa504
force-pushed
the
HDDS-2244
branch
from
5893eb4
to
7a07a16
Compare
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
I have an uber question on this patch. How do we ensure that writes will not be starved on a resource, since Reads allow multiple of them to get thru at the same time? Do we have a mechanism to avoid write starvation in place? if not, it is not better to keep simple locks?
| manager.lock(resourceName); | ||
| LOG.debug("Acquired {} lock on resource {}", resource.name, | ||
| lockFn.accept(resourceName); | ||
| LOG.debug("Acquired {} {} lock on resource {}", lockType, resource.name, | ||
| resourceName); |
There was a problem hiding this comment.
I am trying to read this debug statement.. Do you need to have resource name twice ? once via resource.name and another via resourceName?
There was a problem hiding this comment.
Here the first resource.name prints VOLUME_LOCK/BUCKET_LOCK, next resourceName prints actual resource name. (I think it is little confusing here, because class Resource name is defined like that.
There was a problem hiding this comment.
Yes, it is very confusing. But Thanks for the explanation, it makes sense now.
Right now ActiveLock creates ReadWrite Lock with non-fair mode. Do you mean, we want to create the RWLOCK with an option of fair mode. If my understanding is wrong, could you let me know what additional things need to be implemented? And also this work is mainly to improve read performance workloads, as now with current approach of exclusive lock all reads are serialized. |
When you use a Reader writer lock, there is a question of fairness. Where as exclusive locks are first come first serve.
I am afraid this gives so much importance to Reads that you will have your writes getting stalled completely. |
|
I looked through the JDK implementation of read-write locks a couple of years ago. Even in non-fair mode there is prevention against starvation. HDFS uses non-fair mode by default and works well even for very busy Name Nodes. However we can make the lock fair for now, and evaluate making it non-fair later. |
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
Show resolved
Hide resolved
There was a problem hiding this comment.
+1 LGTM.
Thanks for taking care of this critical improvement @bharatviswa504. Will hold off committing in case @anuengineer has additional comments.
|
+1, I am fine with this getting committed. Thanks for taking care of this issue. |
|
Thank You @anuengineer and @arp7 for the review. For fair/non-fair I will make it configurable. I will open a Jira for this. |
|
Thanks @bharatviswa504 for taking care of this. The change looks good to me. |
Use new ReadWriteLock added in HDDS-2223.
Existing tests should cover this.
Ran a few Integration tests.
Not removed Old methods in OzoneManagerLock, as it is used in Old write requests in VolumeManagerImpl/BucketManagerImpl/KeyManagerImpl. Marked those methods as deprecated.