JyotinderSingh commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r945325588


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -324,6 +345,181 @@ private OMResponse createErrorResponse(
     return omResponse.build();
   }
 
+  /**
+   * Associate bucket ID with requests that have an associated bucket.
+   * Returns a new OMRequest object with the bucket ID associated.
+   *
+   * @param omRequest OMRequest
+   * @return OMRequest with bucket ID associated.
+   * @throws IOException
+   */
+  @SuppressWarnings("checkstyle:MethodLength")
+  private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
+      throws IOException {
+    OMMetadataManager metadataManager = ozoneManager.getMetadataManager();
+    String volumeName = "";
+    String bucketName = "";
+
+    OzoneManagerProtocolProtos.KeyArgs keyArgs;
+    OzoneObj obj;
+    ObjectParser objectParser;
+    ObjectType type;
+
+    switch (omRequest.getCmdType()) {
+    case AddAcl:
+      type = omRequest.getAddAclRequest().getObj().getResType();
+      // No need for bucket ID validation in case of volume ACL
+      if (ObjectType.VOLUME == type) {
+        break;
+      }
+
+      obj =
+          OzoneObjInfo.fromProtobuf(omRequest.getAddAclRequest().getObj());
+      objectParser = new ObjectParser(obj.getPath(), type);
+
+      volumeName = objectParser.getVolume();
+      bucketName = objectParser.getBucket();
+      break;
+    case RemoveAcl:
+      type = omRequest.getAddAclRequest().getObj().getResType();
+      // No need for bucket ID validation in case of volume ACL
+      if (ObjectType.VOLUME == type) {
+        break;
+      }
+
+      obj =
+          OzoneObjInfo.fromProtobuf(omRequest.getRemoveAclRequest().getObj());
+      objectParser = new ObjectParser(obj.getPath(), type);
+
+      volumeName = objectParser.getVolume();
+      bucketName = objectParser.getBucket();
+      break;
+    case SetAcl:
+      type = omRequest.getAddAclRequest().getObj().getResType();
+      // No need for bucket ID validation in case of volume ACL
+      if (ObjectType.VOLUME == type) {
+        break;
+      }
+
+      obj =
+          OzoneObjInfo.fromProtobuf(omRequest.getSetAclRequest().getObj());
+      objectParser = new ObjectParser(obj.getPath(), type);
+
+      volumeName = objectParser.getVolume();
+      bucketName = objectParser.getBucket();
+      break;
+    case DeleteBucket:
+      volumeName = omRequest.getDeleteBucketRequest().getVolumeName();
+      bucketName = omRequest.getDeleteBucketRequest().getBucketName();
+      break;
+    case CreateDirectory:
+      keyArgs = omRequest.getCreateDirectoryRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CreateFile:
+      keyArgs = omRequest.getCreateFileRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CreateKey:
+      keyArgs = omRequest.getCreateKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case AllocateBlock:
+      keyArgs = omRequest.getAllocateBlockRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CommitKey:
+      keyArgs = omRequest.getCommitKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case DeleteKey:
+      keyArgs = omRequest.getDeleteKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case DeleteKeys:
+      OzoneManagerProtocolProtos.DeleteKeyArgs deleteKeyArgs =
+          omRequest.getDeleteKeysRequest()
+              .getDeleteKeys();
+      volumeName = deleteKeyArgs.getVolumeName();
+      bucketName = deleteKeyArgs.getBucketName();
+      break;
+    case RenameKey:
+      keyArgs = omRequest.getRenameKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case RenameKeys:
+      OzoneManagerProtocolProtos.RenameKeysArgs renameKeysArgs =
+          omRequest.getRenameKeysRequest().getRenameKeysArgs();
+      volumeName = renameKeysArgs.getVolumeName();
+      bucketName = renameKeysArgs.getBucketName();
+      break;
+    case InitiateMultiPartUpload:
+      keyArgs = omRequest.getInitiateMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CommitMultiPartUpload:
+      keyArgs = omRequest.getCommitMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case AbortMultiPartUpload:
+      keyArgs = omRequest.getAbortMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CompleteMultiPartUpload:
+      keyArgs = omRequest.getCompleteMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    default:
+      // do nothing in case of other requests.
+      break;
+    }
+
+    // Check if there is any bucket associated with this request.
+    if (bucketName.equals("") && volumeName.equals("")) {
+      return omRequest;
+    }
+
+    long bucketId;
+    try {
+      // Note: Even though this block of code is not executing under a bucket
+      // lock - it is still safe.
+      // For instance, consider the following link bucket chain:
+      // l1 -> l2 -> l3 -> abc (ID: 1000)
+      // Let's say we fetch the resolved bucket name for l1. This would mean
+      // the source bucket is resolved as 'abc'.
+      // Now, let's assume that before the next line of code (to fetch bucket
+      // ID) executes, the link structure changes as follows:
+      // l1- > l2 -> l3 -> xyz (ID: 1001)
+      // And we end up associating the bucket ID for l1 with abc (1000) even
+      // though the actual link has changed.
+      // This is not a problem, since we will anyway be validating this bucket
+      // ID inside validateAndUpdateCache method - and it will be caught there.
+      // This is a fail-slow approach.
+      ResolvedBucket resolvedBucket =
+          ozoneManager.resolveBucketLink(Pair.of(volumeName, bucketName));
+      bucketId = metadataManager.getBucketId(resolvedBucket.realVolume(),
+          resolvedBucket.realBucket());
+    } catch (OMException oe) {
+      // Ignore exceptions at this stage, let respective classes handle them.

Review Comment:
   Done



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -324,6 +345,181 @@ private OMResponse createErrorResponse(
     return omResponse.build();
   }
 
+  /**
+   * Associate bucket ID with requests that have an associated bucket.
+   * Returns a new OMRequest object with the bucket ID associated.
+   *
+   * @param omRequest OMRequest
+   * @return OMRequest with bucket ID associated.
+   * @throws IOException
+   */
+  @SuppressWarnings("checkstyle:MethodLength")
+  private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
+      throws IOException {
+    OMMetadataManager metadataManager = ozoneManager.getMetadataManager();
+    String volumeName = "";
+    String bucketName = "";
+
+    OzoneManagerProtocolProtos.KeyArgs keyArgs;
+    OzoneObj obj;
+    ObjectParser objectParser;
+    ObjectType type;
+
+    switch (omRequest.getCmdType()) {
+    case AddAcl:
+      type = omRequest.getAddAclRequest().getObj().getResType();
+      // No need for bucket ID validation in case of volume ACL
+      if (ObjectType.VOLUME == type) {
+        break;
+      }
+
+      obj =
+          OzoneObjInfo.fromProtobuf(omRequest.getAddAclRequest().getObj());
+      objectParser = new ObjectParser(obj.getPath(), type);
+
+      volumeName = objectParser.getVolume();
+      bucketName = objectParser.getBucket();
+      break;
+    case RemoveAcl:
+      type = omRequest.getAddAclRequest().getObj().getResType();
+      // No need for bucket ID validation in case of volume ACL
+      if (ObjectType.VOLUME == type) {
+        break;
+      }
+
+      obj =
+          OzoneObjInfo.fromProtobuf(omRequest.getRemoveAclRequest().getObj());
+      objectParser = new ObjectParser(obj.getPath(), type);
+
+      volumeName = objectParser.getVolume();
+      bucketName = objectParser.getBucket();
+      break;
+    case SetAcl:
+      type = omRequest.getAddAclRequest().getObj().getResType();
+      // No need for bucket ID validation in case of volume ACL
+      if (ObjectType.VOLUME == type) {
+        break;
+      }
+
+      obj =
+          OzoneObjInfo.fromProtobuf(omRequest.getSetAclRequest().getObj());
+      objectParser = new ObjectParser(obj.getPath(), type);
+
+      volumeName = objectParser.getVolume();
+      bucketName = objectParser.getBucket();
+      break;
+    case DeleteBucket:
+      volumeName = omRequest.getDeleteBucketRequest().getVolumeName();
+      bucketName = omRequest.getDeleteBucketRequest().getBucketName();
+      break;
+    case CreateDirectory:
+      keyArgs = omRequest.getCreateDirectoryRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CreateFile:
+      keyArgs = omRequest.getCreateFileRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CreateKey:
+      keyArgs = omRequest.getCreateKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case AllocateBlock:
+      keyArgs = omRequest.getAllocateBlockRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CommitKey:
+      keyArgs = omRequest.getCommitKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case DeleteKey:
+      keyArgs = omRequest.getDeleteKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case DeleteKeys:
+      OzoneManagerProtocolProtos.DeleteKeyArgs deleteKeyArgs =
+          omRequest.getDeleteKeysRequest()
+              .getDeleteKeys();
+      volumeName = deleteKeyArgs.getVolumeName();
+      bucketName = deleteKeyArgs.getBucketName();
+      break;
+    case RenameKey:
+      keyArgs = omRequest.getRenameKeyRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case RenameKeys:
+      OzoneManagerProtocolProtos.RenameKeysArgs renameKeysArgs =
+          omRequest.getRenameKeysRequest().getRenameKeysArgs();
+      volumeName = renameKeysArgs.getVolumeName();
+      bucketName = renameKeysArgs.getBucketName();
+      break;
+    case InitiateMultiPartUpload:
+      keyArgs = omRequest.getInitiateMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CommitMultiPartUpload:
+      keyArgs = omRequest.getCommitMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case AbortMultiPartUpload:
+      keyArgs = omRequest.getAbortMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    case CompleteMultiPartUpload:
+      keyArgs = omRequest.getCompleteMultiPartUploadRequest().getKeyArgs();
+      volumeName = keyArgs.getVolumeName();
+      bucketName = keyArgs.getBucketName();
+      break;
+    default:
+      // do nothing in case of other requests.

Review Comment:
   Done.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequestUtils.java:
##########
@@ -47,4 +53,50 @@ public static void checkClientRequestPrecondition(
           OMException.ResultCodes.INTERNAL_ERROR);
     }
   }
+
+  /**
+   * Validates the bucket associated with the request - to make sure it did
+   * not change since the request started processing.
+   *
+   * @param bucketId  - bucket ID of the associated bucket when the request
+   *                  is being processed.
+   * @param omRequest - request to be validated, contains the bucket ID of the
+   *                  associated bucket when the request was created.
+   * @throws OMException
+   */
+  public static void validateAssociatedBucketId(long bucketId,
+                                                OMRequest omRequest)
+      throws OMException {
+    if (omRequest.hasAssociatedBucketId()) {
+      if (bucketId != omRequest.getAssociatedBucketId()) {
+        throw new OMException(
+            "Bucket ID mismatch. Associated bucket was modified while this" +

Review Comment:
   Done



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to