You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/11/17 11:59:58 UTC
[GitHub] [ozone] sumitagrawl opened a new pull request, #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
sumitagrawl opened a new pull request, #3973:
URL: https://github.com/apache/ozone/pull/3973
## What changes were proposed in this pull request?
While delete keys in background service for recursive cleanup, cleanup of key's space and its namespace is added in purge of keys.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4565
## How was this patch tested?
1. single file remove with delete folder hierarchy
2. multiple files in different level with delete folder hierarchy
3. multiple files in same level with delete folder hierarchy
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3973:
URL: https://github.com/apache/ozone/pull/3973#discussion_r1027838527
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ try {
+ for (OzoneManagerProtocolProtos.PurgePathRequest path : purgeRequests) {
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getMarkDeletedSubDirsList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getDeletedSubFilesList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedBytes(-sumBlockLengths(keyInfo));
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+ }
+ } catch (IOException ex) {
+ // Case of IOException for fromProtobuf will not hanppen
+ // as this is created and send within OM
+ // only case of upgrade where compatibility is broken can have
+ throw new IllegalStateException(ex);
+ } finally {
+ lockSet.stream().forEach(e -> omMetadataManager.getLock()
+ .releaseWriteLock(BUCKET_LOCK, e.getKey(),
Review Comment:
Looks like the DirectoryDeletingService already has a size limit for the request so this should be ok
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3973:
URL: https://github.com/apache/ozone/pull/3973#discussion_r1027700809
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ try {
+ for (OzoneManagerProtocolProtos.PurgePathRequest path : purgeRequests) {
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getMarkDeletedSubDirsList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getDeletedSubFilesList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedBytes(-sumBlockLengths(keyInfo));
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+ }
+ } catch (IOException ex) {
+ // Case of IOException for fromProtobuf will not hanppen
+ // as this is created and send within OM
+ // only case of upgrade where compatibility is broken can have
+ throw new IllegalStateException(ex);
+ } finally {
+ lockSet.stream().forEach(e -> omMetadataManager.getLock()
+ .releaseWriteLock(BUCKET_LOCK, e.getKey(),
Review Comment:
We are acquiring multiple bucket locks and releasing only when all keys are processed. If the number of keys is large, this can block many reads for a long time . Is there a size limit for number of keys in `purgeDirsRequest.getDeletedPathList()`. I think we should have a size limit.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
Review Comment:
Can we live without `Pair<String,String>` in `Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap`. `OmBucketInfo `already has the `volumeName` and `bucketName `as a field. Instead we can only have a list of OmBucketInfo.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ try {
+ for (OzoneManagerProtocolProtos.PurgePathRequest path : purgeRequests) {
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getMarkDeletedSubDirsList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getDeletedSubFilesList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedBytes(-sumBlockLengths(keyInfo));
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+ }
+ } catch (IOException ex) {
+ // Case of IOException for fromProtobuf will not hanppen
+ // as this is created and send within OM
+ // only case of upgrade where compatibility is broken can have
+ throw new IllegalStateException(ex);
+ } finally {
+ lockSet.stream().forEach(e -> omMetadataManager.getLock()
+ .releaseWriteLock(BUCKET_LOCK, e.getKey(),
+ e.getValue()));
+ for (Map.Entry<Pair<String, String>, OmBucketInfo> entry :
+ volBucketInfoMap.entrySet()) {
+ entry.setValue(entry.getValue().copyObject());
Review Comment:
Not sure I understood this correctly, Why are we copying object here?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #3973:
URL: https://github.com/apache/ozone/pull/3973#discussion_r1027842754
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
Review Comment:
We check and add lock or bucketInfo only once, so map is required with key as volume and bucket.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #3973:
URL: https://github.com/apache/ozone/pull/3973#discussion_r1027841658
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ try {
+ for (OzoneManagerProtocolProtos.PurgePathRequest path : purgeRequests) {
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getMarkDeletedSubDirsList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+
+ for (OzoneManagerProtocolProtos.KeyInfo key :
+ path.getDeletedSubFilesList()) {
+ OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key);
+ String volumeName = keyInfo.getVolumeName();
+ String bucketName = keyInfo.getBucketName();
+ Pair<String, String> volBucketPair = Pair.of(volumeName, bucketName);
+ if (!lockSet.contains(volBucketPair)) {
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
+ volumeName, bucketName);
+ lockSet.add(volBucketPair);
+ }
+ OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager,
+ volumeName, bucketName);
+ // bucketInfo can be null in case of delete volume or bucket
+ if (null != omBucketInfo) {
+ omBucketInfo.incrUsedBytes(-sumBlockLengths(keyInfo));
+ omBucketInfo.incrUsedNamespace(-1L);
+ volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo);
+ }
+ }
+ }
+ } catch (IOException ex) {
+ // Case of IOException for fromProtobuf will not hanppen
+ // as this is created and send within OM
+ // only case of upgrade where compatibility is broken can have
+ throw new IllegalStateException(ex);
+ } finally {
+ lockSet.stream().forEach(e -> omMetadataManager.getLock()
+ .releaseWriteLock(BUCKET_LOCK, e.getKey(),
+ e.getValue()));
+ for (Map.Entry<Pair<String, String>, OmBucketInfo> entry :
+ volBucketInfoMap.entrySet()) {
+ entry.setValue(entry.getValue().copyObject());
Review Comment:
Followed similar logic as other place, before add to response, copy the bucketInfo
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3973:
URL: https://github.com/apache/ozone/pull/3973#discussion_r1027845437
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -48,12 +59,72 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
+ Set<Pair<String, String>> lockSet = new HashSet<>();
+ Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
Review Comment:
From offline discussion: There might be multiple keys from same bucket which can cause duplicate entries in the list.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 merged pull request #3973: HDDS-4565. [FSO]Delete : quotaReleased on directory deletion should consider all sub paths
Posted by GitBox <gi...@apache.org>.
sadanand48 merged PR #3973:
URL: https://github.com/apache/ozone/pull/3973
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org