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