HDDS-13723. Add detailed lock information for omMetadataManager locks#9157
HDDS-13723. Add detailed lock information for omMetadataManager locks#9157ivandika3 merged 4 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
@sreejasahithi Thanks for the patch.
The main point of adding detailed lock information is for OM RPC FairCallQueue calculation (whether a particular UGI should be put to lower priority call queue if a lot of the RPC calls hold locks). However these are only effective if the request is sent through the OM RPC server (see OzoneManagerProtocolServerSideTranslatorPB#processRequest) and since OMDirectoriesPurgeRequestWithFSO and S3ExpiredMultipartUploadsAbortRequest are sent to Ratis server directly, merging the OM lock details might not have any noticeable effect.
Thanks for the review, @ivandika3 and for pointing this out. @jojochuang, what are your thoughts on this? |
|
@ivandika3 thanks for the clarification. That being said, a number of request types are also issued via OM Ratis directly (e.g. PurgeKeys, PurgeDirectories) but their handlers do record detailed OM lock information. Ok. Looks like we can just close the PR as it is intended. |
@jojochuang Yes, I think these requests follow the current convention. IMO I'm OK if we go ahead with this change as a way to standardize the usage of locks (the overhead should not be that large). We can extract these locking methods instead. In the future, we might want to defer the background service in another process and the OM lock details. |
|
Thanks @ivandika3 , @jojochuang, Since the goal is to standardize usage of locks across all request types, I’ll keep the current change. Let me know if there are any more comments. |
|
@sreejasahithi Thanks for the patch, left some comments. Could you help double check whether there are other requests with similar situations? |
| entry.setValue(entry.getValue().copyObject()); | ||
| } | ||
| omMetadataManager.getLock().releaseWriteLocks(BUCKET_LOCK, bucketLockKeys); | ||
| mergeOmLockDetails(omMetadataManager.getLock().releaseWriteLocks(BUCKET_LOCK, bucketLockKeys)); |
There was a problem hiding this comment.
I think we should only call releaseWriteLocks, if isLockAcquired is set to true (similar situation in OmKeyPurgeRequest). @swamirishi please help to advise here, I'm not familiar with the new bulk locks feature.
sumitagrawl
left a comment
sumitagrawl
left a comment
There was a problem hiding this comment.
@sreejasahithi given minor comment
...ne/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java
Show resolved
Hide resolved
|
Thanks @sreejasahithi for the patch and @sumitagrawl , @jojochuang for the reviews. |
5 participants
What changes were proposed in this pull request?
HDDS-8974 introduced detailed lock information for almost all operations acquiring omMetadataManager locks by wrapping the lock acquisition with mergeOmLockDetails and tracking lockAcquired status.
However, a few places appear to have missed this pattern, including
S3ExpiredMultipartUploadsAbortRequestandOMDirectoriesPurgeRequestWithFSO.What is the link to the Apache JIRA
HDDS-13723
How was this patch tested?
Geen CI : https://github.com/sreejasahithi/ozone/actions/runs/18500762135