You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "jojochuang (via GitHub)" <gi...@apache.org> on 2023/06/03 01:03:09 UTC

[GitHub] [ozone] jojochuang opened a new pull request, #4825: Hdds 8436

jojochuang opened a new pull request, #4825:
URL: https://github.com/apache/ozone/pull/4825

   ## What changes were proposed in this pull request?
   
   Implement new Hadoop file system APIs to support HBase.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-8436 Support setSafeMode(), isFileClosed() FileSystem API
   
   
   ## How was this patch tested?
   Unit tests


-- 
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] szetszwo commented on a diff in pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4825:
URL: https://github.com/apache/ozone/pull/4825#discussion_r1245368336


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -4431,6 +4433,24 @@ public void setTimes(OmKeyArgs keyArgs, long mtime, long atime)
       throws IOException {
   }
 
+  @Override
+  public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+      throws IOException {
+    switch (action) {
+    case ENTER:
+      throw new OMException("Enter safe mode is unsupported",
+          INTERNAL_ERROR);
+    case LEAVE:
+    case FORCE_EXIT:

Review Comment:
   Currently, we only support `FORCE_EXIT`.  We should support `LEAVE` later.



##########
hadoop-ozone/ozonefs-hadoop3/src/main/java/org/apache/hadoop/fs/ozone/OzoneFileSystem.java:
##########
@@ -120,4 +123,28 @@ public boolean hasPathCapability(final Path path, final String capability)
     }
     return super.hasPathCapability(p, capability);
   }
+
+  @Override
+  public boolean recoverLease(Path f) throws IOException {
+    LOG.trace("recoverLease() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().recoverLease(key);
+  }
+
+  @Override
+  public boolean isFileClosed(Path f) throws IOException {
+    LOG.trace("isFileClosed() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().isFileClosed(key);
+  }
+
+  @Override
+  public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+      throws IOException {
+    statistics.incrementWriteOps(1);

Review Comment:
   Check if the action is not `GET` before `incrementWriteOps`.



##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java:
##########
@@ -716,9 +717,37 @@ private SnapshotDiffReportOzone getSnapshotDiffReportOnceComplete(
     return snapshotDiffResponse.getSnapshotDiffReport();
   }
 
+  @Override
+  public boolean recoverLease(final String pathStr) throws IOException {
+    incrementCounter(Statistic.INVOCATION_RECOVER_LEASE, 1);
+
+    return ozoneClient.getProxy().getOzoneManagerClient().recoverLease(
+        volume.getName(), bucket.getName(), pathStr);
+  }
+
   @Override
   public void setTimes(String key, long mtime, long atime) throws IOException {
     incrementCounter(Statistic.INVOCATION_SET_TIMES, 1);
     bucket.setTimes(key, mtime, atime);
   }
+
+  @Override
+  public boolean isFileClosed(String pathStr) throws IOException {
+    incrementCounter(Statistic.INVOCATION_IS_FILE_CLOSED, 1);
+    OFSPath ofsPath = new OFSPath(pathStr, config);
+    if (!ofsPath.isKey()) {
+      throw new IOException("not a file");
+    }
+    OzoneFileStatus status = bucket.getFileStatus(pathStr);

Review Comment:
   We should check `status.isFile()`.



##########
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/OzoneFileSystem.java:
##########
@@ -120,4 +123,28 @@ public boolean hasPathCapability(final Path path, final String capability)
     }
     return super.hasPathCapability(p, capability);
   }
+
+  @Override
+  public boolean recoverLease(Path f) throws IOException {
+    LOG.trace("isFileClosed() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().recoverLease(key);
+  }
+
+  @Override
+  public boolean isFileClosed(Path f) throws IOException {
+    LOG.trace("isFileClosed() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().isFileClosed(key);
+  }
+
+  @Override
+  public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+      throws IOException {
+    statistics.incrementWriteOps(1);

Review Comment:
   Check if the action is not `GET` before `incrementWriteOps`.



##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1370,20 +1371,50 @@ private SnapshotDiffReportOzone getSnapshotDiffReportOnceComplete(
     return snapshotDiffResponse.getSnapshotDiffReport();
   }
 
-  public boolean recoverLease(final Path f) throws IOException {
-    OFSPath ofsPath = new OFSPath(f, config);
+  @Override
+  public boolean isFileClosed(String pathStr) throws IOException {
+    incrementCounter(Statistic.INVOCATION_IS_FILE_CLOSED, 1);
+    OFSPath ofsPath = new OFSPath(pathStr, config);
+    String key = ofsPath.getKeyName();
+    if (ofsPath.isRoot() || ofsPath.isVolume()) {
+      throw new IOException("not a file");
+    } else {
+      OzoneBucket bucket = getBucket(ofsPath, false);
+      if (ofsPath.isSnapshotPath()) {
+        throw new IOException("file is in a snapshot.");
+      } else {
+        OzoneFileStatus status = bucket.getFileStatus(key);

Review Comment:
   Check `status.isFile()`.



##########
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/RootedOzoneFileSystem.java:
##########
@@ -118,4 +121,30 @@ public boolean hasPathCapability(final Path path, final String capability)
     }
     return super.hasPathCapability(p, capability);
   }
+
+  @Override
+  public boolean recoverLease(final Path f) throws IOException {
+    statistics.incrementWriteOps(1);
+    LOG.trace("recoverLease() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().recoverLease(key);
+  }
+
+  @Override
+  public boolean isFileClosed(Path f) throws IOException {
+    statistics.incrementWriteOps(1);
+    LOG.trace("isFileClosed() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().isFileClosed(key);
+  }
+
+  @Override
+  public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+      throws IOException {
+    statistics.incrementWriteOps(1);

Review Comment:
   Check if the action is not `GET` before `incrementWriteOps`.



-- 
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] szetszwo commented on a diff in pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4825:
URL: https://github.com/apache/ozone/pull/4825#discussion_r1228882857


##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -135,6 +135,14 @@ enum Type {
   SnapshotPurge = 118;
   RecoverLease = 119;
   SetTimes = 120;
+  SetSafeMode = 121;

Review Comment:
   @jojochuang , 121 is already taken in the master branch.  Please sync this with the master branch.
   ```
   <<<<<<< HEAD
     RefetchSecretKey = 121;
   =======
     SetSafeMode = 121;
   >>>>>>> jojochuang/HDDS-8436
   ```



-- 
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] szetszwo commented on a diff in pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4825:
URL: https://github.com/apache/ozone/pull/4825#discussion_r1245441994


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -4431,6 +4433,24 @@ public void setTimes(OmKeyArgs keyArgs, long mtime, long atime)
       throws IOException {
   }
 
+  @Override
+  public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+      throws IOException {
+    switch (action) {
+    case ENTER:
+      throw new OMException("Enter safe mode is unsupported",
+          INTERNAL_ERROR);
+    case LEAVE:
+    case FORCE_EXIT:

Review Comment:
   For `LEAVE`, it should exit safe mode only if preCheck is true.  We should make the change separately.
   ```java
   //SCMSafeModeManager
   @@ -237,20 +237,25 @@ public void completePreCheck(EventPublisher eventQueue) {
       * 2. Emits START_REPLICATION for ReplicationManager.
       * 3. Cleanup resources.
       * 4. Emit safe mode status.
   -   * @param eventQueue
       */
      @VisibleForTesting
   -  public void exitSafeMode(EventPublisher eventQueue) {
   -    LOG.info("SCM exiting safe mode.");
   -    // If safemode is exiting, then pre check must also have passed so
   -    // set it to true.
   -    setPreCheckComplete(true);
   +  public boolean exitSafeMode(boolean force) {
   +    LOG.info("SCM exiting safe mode, force? {}", force);
   +    if (force) {
   +      // If force exiting, set preCheck to true.
   +      setPreCheckComplete(true);
   +    } else {
   +      if (!getPreCheckComplete()) {
   +        return false;
   +      }
   +    }
        setInSafeMode(false);
    
        // TODO: Remove handler registration as there is no need to listen to
        // register events anymore.
    
        emitSafeModeStatus();
   +    return true;
      }
   ```



-- 
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] szetszwo commented on a diff in pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4825:
URL: https://github.com/apache/ozone/pull/4825#discussion_r1246240173


##########
hadoop-ozone/ozonefs-hadoop3/src/main/java/org/apache/hadoop/fs/ozone/RootedOzoneFileSystem.java:
##########
@@ -118,4 +121,39 @@ public boolean hasPathCapability(final Path path, final String capability)
     }
     return super.hasPathCapability(p, capability);
   }
+
+  /**
+   * Start the lease recovery of a file.
+   *
+   * @param f a file
+   * @return true if the file is already closed
+   * @throws IOException if an error occurs
+   */
+  @Override
+  public boolean recoverLease(final Path f) throws IOException {
+    incrementCounter(Statistic.INVOCATION_RECOVER_LEASE, 1);
+    statistics.incrementWriteOps(1);
+    LOG.trace("recoverLease() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().recoverLease(key);
+  }
+
+  @Override
+  public boolean isFileClosed(Path f) throws IOException {
+    incrementCounter(Statistic.INVOCATION_IS_FILE_CLOSED, 1);
+    statistics.incrementReadOps(1);
+    LOG.trace("isFileClosed() path:{}", f);
+    Path qualifiedPath = makeQualified(f);
+    String key = pathToKey(qualifiedPath);
+    return getAdapter().isFileClosed(key);
+  }
+
+  @Override
+  public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+      throws IOException {
+    statistics.incrementWriteOps(1);

Review Comment:
   Check if the action is not `GET` before `incrementWriteOps`.



-- 
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] szetszwo merged pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #4825:
URL: https://github.com/apache/ozone/pull/4825


-- 
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] jojochuang commented on pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #4825:
URL: https://github.com/apache/ozone/pull/4825#issuecomment-1613844294

   But as it turns out, we test MR jobs with Hadoop 3.1 3.2 and 3.3 runtime, and the MR job tests fail with Hadoop 3.1 and 3.2 (due to the lacking of SafeMode and LeaseRecoverable interface). So either we find a workaround or remove the Hadoop 3.1 3.2 tests.


-- 
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] jojochuang commented on pull request #4825: HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #4825:
URL: https://github.com/apache/ozone/pull/4825#issuecomment-1613841424

   > BTW, is it possible to move the setSafeMode method to Basic RootedOzoneFileSystem?
   
   We can't, because Hadoop 2 does not have SafeMode interface (introduced in Hadoop 3.3.6). So I can only put it where Ozone is not run with Hadoop 2 runtime.
   


-- 
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