-
Notifications
You must be signed in to change notification settings - Fork 537
HDDS-12356. granular locking framework [rework in split] #7936
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
@sumitagrawl , could we do the refactoring first and then add the new code? It is hard to review when refactoring and new code are mixing together. |
@szetszwo
There is no major refactoring here. |
@sumitagrawl , Why mixing code refactoring with new code in such a big and non-trivial change? It is fine for small changes but this is not. It is actually not easy to tell if the new code is being used in the existing code. |
...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/WrappedStripedLock.java
Show resolved
Hide resolved
/** | ||
* Manage locking of volume, bucket and keys. | ||
*/ | ||
public class OmLockOpr { |
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.
What does Opr
mean?
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.
Opr refers to operation
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.
Let's rename it to OmLockOperation
. Otherwise, people will keep asking the same question.
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.
@sumitagrawl , thanks for the update! I have two quick comments inlined.
/** | ||
* Manage locking of volume, bucket and keys. | ||
*/ | ||
public class OmLockOpr { |
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.
Let's rename it to OmLockOperation
. Otherwise, people will keep asking the same question.
@@ -36,10 +40,12 @@ public class OMExecutionFlow { | |||
|
|||
private final OzoneManager ozoneManager; | |||
private final OMPerformanceMetrics perfMetrics; | |||
private final OmLockOpr omLockOpr; |
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.
Let's don't change OMExecutionFlow and OMClientRequest in this PR. It is good to make it clear that this PR is not changing the existing code.
What changes were proposed in this pull request?
Granular locking framework for OBS for existing flow. Its just framework binding code flow but pending to call the lock in respective flow.
This also involve refactoring of request handling moving few common parts to OMGateway wrt to new flow in prototype.
locking is added for external request at entry point. This provides execution of request at leader and existing flow simultaneouly without impacting for cache.
locking added for obs request
(HDDS-11898. design doc leader side execution)Step-by-step integration
of existing request (interoperability)Next PR will include:
Parent Jira:
https://issues.apache.org/jira/browse/HDDS-11900
Its Parent for Epic;
https://issues.apache.org/jira/browse/HDDS-11897
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11900
How was this patch tested?