[GitHub] [ozone] avijayanhwx commented on a change in pull request #2265: HDDS-5244. Allow multiple OM request versions to be supported at same layout version.

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

avijayanhwx commented on a change in pull request #2265:
URL: https://github.com/apache/ozone/pull/2265#discussion_r638111409



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/upgrade/OMLayoutFeatureAspect.java
##########
@@ -63,16 +68,47 @@ public void checkLayoutFeature(JoinPoint joinPoint) throws Throwable {
         lvm = new OMLayoutVersionManager();
       }
     }
+    checkIsAllowed(joinPoint.getSignature().toShortString(), lvm, featureName);
+  }
+
+  private void checkIsAllowed(String operationName,
+                              LayoutVersionManager lvm,
+                              String featureName) throws OMException {
     if (!lvm.isAllowed(featureName)) {
       LayoutFeature layoutFeature = lvm.getFeature(featureName);
       throw new OMException(String.format("Operation %s cannot be invoked " +
               "before finalization. It belongs to the layout feature %s, " +
               "whose layout version is %d. Current Layout version is %d",
-          joinPoint.getSignature().toShortString(),
+          operationName,
           layoutFeature.name(),
           layoutFeature.layoutVersion(),
           lvm.getMetadataLayoutVersion()),
           NOT_SUPPORTED_OPERATION);
     }
   }
+
+  @Pointcut("execution(* " +
+      "org.apache.hadoop.ozone.om.request.OMClientRequest+.preExecute(..)) " +
+      "&& @this(org.apache.hadoop.ozone.om.upgrade.BelongsToLayoutVersion)")
+  public void omRequestPointCut() {
+  }
+
+  @Before("omRequestPointCut()")
+  public void beforeRequestApplyTxn(final JoinPoint joinPoint)

Review comment:
       To bring in the FSO through upgrades, we would need to
   
   1. Add a new Layout feature in org.apache.hadoop.ozone.om.upgrade.OMLayoutFeature
   2. To make sure new feature is not used in finalization, we can do either of the following
       - add the @BelongsToLayoutVersion to all the new FSO request classes referencing the above layout feature
       - Create a prefinalize validation action like org.apache.hadoop.hdds.scm.server.upgrade.ScmHAUnfinalizedStateValidationAction to make sure one cannot enable FSO layout version before finalization.
   cc @errose28 

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java
##########
@@ -58,110 +82,133 @@
  */
 public final class OzoneManagerRatisUtils {
 
-  private static final Logger LOG = LoggerFactory
-      .getLogger(OzoneManagerRatisUtils.class);
-
   private OzoneManagerRatisUtils() {
   }
-
-  public static OMClientRequest getRequest(OzoneManager om,
-                                           OMRequest omRequest) {
+  /**
+   * Create OMClientRequest which encapsulates the OMRequest.
+   * @param omRequest
+   * @return OMClientRequest
+   * @throws IOException
+   */
+  public static OMClientRequest createClientRequest(OMRequest omRequest) {
     Type cmdType = omRequest.getCmdType();
     switch (cmdType) {
+    case CreateVolume:
+      return new OMVolumeCreateRequest(omRequest);
+    case SetVolumeProperty:
+      boolean hasQuota = omRequest.getSetVolumePropertyRequest()
+          .hasQuotaInBytes();
+      boolean hasOwner = omRequest.getSetVolumePropertyRequest().hasOwnerName();
+      Preconditions.checkState(hasOwner || hasQuota, "Either Quota or owner " +
+          "should be set in the SetVolumeProperty request");
+      Preconditions.checkState(!(hasOwner && hasQuota), "Either Quota or " +
+          "owner should be set in the SetVolumeProperty request. Should not " +
+          "set both");
+      if (hasQuota) {
+        return new OMVolumeSetQuotaRequest(omRequest);
+      } else {
+        return new OMVolumeSetOwnerRequest(omRequest);
+      }
+    case DeleteVolume:
+      return new OMVolumeDeleteRequest(omRequest);
+    case CreateBucket:
+      return new OMBucketCreateRequest(omRequest);
+    case DeleteBucket:
+      return new OMBucketDeleteRequest(omRequest);
+    case SetBucketProperty:
+      return new OMBucketSetPropertyRequest(omRequest);
+    case AllocateBlock:
+      return new OMAllocateBlockRequest(omRequest);
+    case CreateKey:
+      return new OMKeyCreateRequest(omRequest);
+    case CommitKey:
+      return new OMKeyCommitRequest(omRequest);
+    case DeleteKey:
+      return new OMKeyDeleteRequest(omRequest);
+    case DeleteKeys:
+      return new OMKeysDeleteRequest(omRequest);
+    case RenameKey:
+      return new OMKeyRenameRequest(omRequest);
+    case RenameKeys:
+      return new OMKeysRenameRequest(omRequest);
+    case CreateDirectory:
+      return new OMDirectoryCreateRequest(omRequest);
+    case CreateFile:
+      return new OMFileCreateRequest(omRequest);

Review comment:
       @rakeshadr Before this patch, there were 2 OMRequest use cases that were handled by the Upgrade framework.
   
   - Framework provides the correct version of the OM Request class that is part of the current layout version. This assumed that for a given layout version, only 1 version of OM request class can exist. Since the changes done for HDDS-2939 made that untrue, we have dropped this handling for now, and punted into the future based on how OMRequest hierarchy is evolving.
   - Framework makes sure new OM requests are not invoked before finalizing the layout version they are introduced in. This has been retained through the @BelongsToLayoutVersion annotation that can be added at the class level.




-- 
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org