[
https://issues.apache.org/jira/browse/HADOOP-18671?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17718559#comment-17718559
]
ASF GitHub Bot commented on HADOOP-18671:
-----------------------------------------
steveloughran commented on code in PR #5553:
URL: https://github.com/apache/hadoop/pull/5553#discussion_r1182489638
##########
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/safemode.md:
##########
@@ -0,0 +1,45 @@
+<!---
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License. See accompanying LICENSE file.
+-->
+
+# <a name="SafeMode"></a> interface `SafeMode`
+
+The `SafeMode` interface provides a way to perform safe mode actions and
obtain the
+status after such actions performed to the `FileSystem`.
+
+This is admin only interface, should be implemented accordingly when necessary
to
+Filesystem that support safe mode, e.g. `DistributedFileSystem` (HDFS) and
+`ViewDistributedFileSystem`.
+
+```java
+public interface SafeMode {
+ default boolean setSafeMode(SafeModeAction action) throws IOException {
+ return setSafeMode(action, false);
+ }
+ boolean setSafeMode(SafeModeAction action, boolean isChecked) throws
IOException;
+}
+```
+
+The goals of this interface is allow any file system implementation to share
the
+same concept of safe mode with the following actions and states
+
+Safe mode actions
+1. `GET`, get the safe mode status of the file system.
+1. `ENTER`, enter the safe mode for the file system.
+1. `LEAVE`, exit safe mode for the file system gracefully.
+1. `FORCE_EXIT`, exit safe mode for the file system even if there is any
ongoing data process.
+
+Safe mode states
+1. `ON`, when safe mode is on.
Review Comment:
true/false from the return values, not on/off
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/contract/hdfs/TestHDFSContractSafeMode.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.contract.hdfs;
+
+import org.apache.hadoop.conf.Configuration;
Review Comment:
import ordering
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestLeaseManager.java:
##########
@@ -24,14 +24,15 @@
import org.apache.hadoop.fs.FSDataOutputStream;
import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.SafeModeAction;
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.fs.permission.PermissionStatus;
import org.apache.hadoop.hdfs.DFSUtil;
import org.apache.hadoop.hdfs.DistributedFileSystem;
import org.apache.hadoop.hdfs.HdfsConfiguration;
import org.apache.hadoop.hdfs.MiniDFSCluster;
import org.apache.hadoop.hdfs.protocol.HdfsConstants;
-import org.apache.hadoop.hdfs.protocol.HdfsConstants.SafeModeAction;
+
Review Comment:
cut
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractLeaseRecoveryTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.contract;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.LeaseRecoverable;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.test.LambdaTestUtils;
+
+import org.assertj.core.api.Assertions;
Review Comment:
this block MUST go *above* the org.apache block
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgrade.java:
##########
@@ -31,11 +31,12 @@
import java.io.IOException;
import java.util.regex.Pattern;
+import org.apache.hadoop.fs.SafeModeAction;
Review Comment:
put in right place in the org.apache import.
##########
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/safemode.md:
##########
@@ -0,0 +1,45 @@
+<!---
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License. See accompanying LICENSE file.
+-->
+
+# <a name="SafeMode"></a> interface `SafeMode`
+
+The `SafeMode` interface provides a way to perform safe mode actions and
obtain the
+status after such actions performed to the `FileSystem`.
+
+This is admin only interface, should be implemented accordingly when necessary
to
+Filesystem that support safe mode, e.g. `DistributedFileSystem` (HDFS) and
+`ViewDistributedFileSystem`.
+
+```java
+public interface SafeMode {
+ default boolean setSafeMode(SafeModeAction action) throws IOException {
+ return setSafeMode(action, false);
+ }
+ boolean setSafeMode(SafeModeAction action, boolean isChecked) throws
IOException;
+}
+```
+
+The goals of this interface is allow any file system implementation to share
the
+same concept of safe mode with the following actions and states
+
+Safe mode actions
+1. `GET`, get the safe mode status of the file system.
+1. `ENTER`, enter the safe mode for the file system.
+1. `LEAVE`, exit safe mode for the file system gracefully.
+1. `FORCE_EXIT`, exit safe mode for the file system even if there is any
ongoing data process.
+
+Safe mode states
+1. `ON`, when safe mode is on.
+1. `OFF`, when safe mode is off, usually it's the result of safe mode actions
+with `GET`, `LEAVE`, `FORCE_EXIT`
Review Comment:
add some detail on using hasPathCapability to probe for the appropriate
capability before execution; with path as root of the FS.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestViewDistributedFileSystem.java:
##########
@@ -195,18 +202,103 @@ public void testQuota() throws IOException {
@Test
public void testPathCapabilities() throws IOException {
- Configuration conf = getTestConfiguration();
- try (MiniDFSCluster cluster = new
MiniDFSCluster.Builder(conf).numDataNodes(0).build()) {
- URI defaultUri =
URI.create(conf.get(CommonConfigurationKeys.FS_DEFAULT_NAME_KEY));
- conf.set("fs.viewfs.mounttable." + defaultUri.getHost() +
".linkFallback",
- defaultUri.toString());
- try (ViewDistributedFileSystem fileSystem = (ViewDistributedFileSystem)
FileSystem.get(
- conf)) {
- final Path testFile = new Path("/test");
- assertTrue("ViewDfs supports truncate",
- fileSystem.hasPathCapability(testFile,
CommonPathCapabilities.FS_TRUNCATE));
- }
+ try (MiniDFSCluster cluster = new
MiniDFSCluster.Builder(getViewFsConfiguration())
+ .numDataNodes(0).build();
+ ViewDistributedFileSystem fileSystem = (ViewDistributedFileSystem)
FileSystem.get(
+ cluster.getConfiguration(0))) {
+ final Path testFile = new Path("/test");
+ assertTrue("ViewDfs supports truncate",
+ fileSystem.hasPathCapability(testFile,
CommonPathCapabilities.FS_TRUNCATE));
+ final boolean isLeaseRecoverable =
fileSystem.hasPathCapability(testFile, LEASE_RECOVERABLE);
+ assertThat(isLeaseRecoverable).describedAs("path capabilities %s=%s in
%s",
+ LEASE_RECOVERABLE, fileSystem.hasPathCapability(testFile,
LEASE_RECOVERABLE),
+ fileSystem).isTrue();
+ assertThat(fileSystem).describedAs("filesystem %s", fileSystem)
+ .isInstanceOf(LeaseRecoverable.class);
+ assertThat(fileSystem).describedAs("filesystem %s",
fileSystem).isInstanceOf(SafeMode.class);
+ }
+ }
+
+ @Test
+ public void testSafeMode() throws IOException {
+ testSafeMode(this::executeAssertionsWithSafeMode);
+ }
+
+ @Test
+ public void testSafeModeWithDeprecatedAPIs() throws IOException {
+ testSafeMode(this::executeAssertionsWithDeprecatedAPIs);
+ }
+
+ private void testSafeMode(Consumer<ViewDistributedFileSystem>
executeAssertionsFunction)
+ throws IOException {
+ try (MiniDFSCluster cluster = new
MiniDFSCluster.Builder(getViewFsConfiguration())
+ .numDataNodes(0).build();
+ ViewDistributedFileSystem fileSystem = (ViewDistributedFileSystem)
FileSystem.get(
+ cluster.getConfiguration(0))) {
+ executeAssertionsFunction.accept(fileSystem);
}
}
+ private SafeMode verifyAndGetSafeModeInstance(FileSystem fs) {
+ Assertions.assertThat(fs)
+ .describedAs("File system %s must be an instance of %s", fs,
SafeMode.class.getClass())
+ .isInstanceOf(SafeMode.class);
+ return (SafeMode) fs;
+ }
+
+ private void executeAssertionsWithSafeMode(ViewDistributedFileSystem
fileSystem) {
+ SafeMode fsWithSafeMode = verifyAndGetSafeModeInstance(fileSystem);
+ assertSafeModeStatus(fsWithSafeMode, SafeModeAction.GET, false,
+ "Getting the status of safe mode before entering should be off.");
+ assertSafeModeStatus(fsWithSafeMode, SafeModeAction.ENTER, true,
+ "Entering Safe mode and safe mode turns on.");
+ assertSafeModeStatus(fsWithSafeMode, SafeModeAction.GET, true,
+ "Getting the status of safe mode after entering, safe mode should be
on.");
+ assertSafeModeStatus(fsWithSafeMode, SafeModeAction.LEAVE, false,
+ "Leaving safe mode, and safe mode switches off.");
+ assertSafeModeStatus(fsWithSafeMode, SafeModeAction.FORCE_EXIT, false,
+ "Force exist safe mode at any time, safe mode should always switches
off.");
+ }
+
+ private void executeAssertionsWithDeprecatedAPIs(ViewDistributedFileSystem
fileSystem) {
+ assertSafeModeStatus(fileSystem,
HdfsConstants.SafeModeAction.SAFEMODE_GET, false,
+ "Getting the status of safe mode before entering should be off.");
+ assertSafeModeStatus(fileSystem,
HdfsConstants.SafeModeAction.SAFEMODE_ENTER, true,
+ "Entering Safe mode and safe mode turns on.");
+ assertSafeModeStatus(fileSystem,
HdfsConstants.SafeModeAction.SAFEMODE_GET, true,
+ "Getting the status of safe mode after entering, safe mode should be
on.");
+ assertSafeModeStatus(fileSystem,
HdfsConstants.SafeModeAction.SAFEMODE_LEAVE, false,
+ "Leaving safe mode, and safe mode switches off.");
+ assertSafeModeStatus(fileSystem,
HdfsConstants.SafeModeAction.SAFEMODE_FORCE_EXIT, false,
+ "Force exist safe mode at any time, safe mode should always switches
off.");
+ }
+
+ private void assertSafeModeStatus(SafeMode fsWithSafeMode, SafeModeAction
action,
+ boolean expectedStatus, String message) {
+ try {
+
Assertions.assertThat(fsWithSafeMode.setSafeMode(action)).describedAs(message)
+ .isEqualTo(expectedStatus);
+ } catch (Exception e) {
Review Comment:
Is the problem the IOE that safemode raises? As using
`org.apache.hadoop.util.functional.ConsumerRaisingIOE` as your consumer means
you can avoid the need to catch and wrap.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestRollingUpgradeRollback.java:
##########
@@ -22,8 +22,9 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.SafeModeAction;
import org.apache.hadoop.hdfs.protocol.HdfsConstants.RollingUpgradeAction;
-import org.apache.hadoop.hdfs.protocol.HdfsConstants.SafeModeAction;
+
Review Comment:
delete line
> Add recoverLease(), setSafeMode(), isFileClosed() APIs to FileSystem
> --------------------------------------------------------------------
>
> Key: HADOOP-18671
> URL: https://issues.apache.org/jira/browse/HADOOP-18671
> Project: Hadoop Common
> Issue Type: New Feature
> Components: fs
> Reporter: Wei-Chiu Chuang
> Priority: Major
> Labels: pull-request-available
>
> We are in the midst of enabling HBase and Solr to run on Ozone.
> An obstacle is that HBase relies heavily on HDFS APIs and semantics for its
> Write Ahead Log (WAL) file (similarly, for Solr's transaction log). We
> propose to push up these HDFS APIs, i.e. recoverLease(), setSafeMode(),
> isFileClosed() to FileSystem abstraction so that HBase and other applications
> do not need to take on Ozone dependency at compile time. This work will
> (hopefully) enable HBase to run on other storage system implementations in
> the future.
> There are other HDFS features that HBase uses, including hedged read and
> favored nodes. Those are FS-specific optimizations and are not critical to
> enable HBase on Ozone.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
