[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.

[GitBox <gi...@apache.org>]

smengcl commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451240368



##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
 }
 
 message DeleteKeysResponse {
-    repeated KeyInfo deletedKeys = 1;
-    repeated KeyInfo unDeletedKeys = 2;

Review comment:
       `unDeletedKeys` seems to be added in this [comment](https://github.com/apache/hadoop-ozone/pull/814/files#r429342829) on purpose.
   
   Will removing those two fields cause any compatibility issue?

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
     OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
         getOmRequest());
     OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+    // As right now, only client exposed API is for a single volume and
+    // bucket. So, all entries will have same volume name and bucket name.
+    // So, we can validate once.
+    if (deleteKeyArgsList.size() > 0) {
+      volumeName = deleteKeyArgsList.get(0).getVolumeName();
+      bucketName = deleteKeyArgsList.get(0).getBucketName();
+    }
+
+    boolean acquiredLock =
+        omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+            bucketName);
+
+    int indexFailed = 0;
     try {
-      for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+      // Validate bucket and volume exists or not.
+      if (deleteKeyArgsList.size() > 0) {
+        validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+      }
+
+
+      // Check if any of the key in the batch cannot be deleted. If exists the
+      // batch delete will be failed.
+
+      for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+           indexFailed++) {
+        KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);

Review comment:
       Why do we always get the first element here? Typo?
   
   Also, I didn't find existing test for `OMKeysDeleteRequest` or `OMKeysDeleteResponse`. It'd be a good idea to add some tests for sanity checks.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org