steveloughran commented on code in PR #5553:
URL: https://github.com/apache/hadoop/pull/5553#discussion_r1175271342
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonPathCapabilities.java:
##########
@@ -163,5 +163,7 @@ private CommonPathCapabilities() {
public static final String ETAGS_PRESERVED_IN_RENAME =
"fs.capability.etags.preserved.in.rename";
+ public static final String LEASE_RECOVERABLE =
Review Comment:
add javadocs
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractLeaseRecoveryTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 static org.apache.hadoop.fs.CommonPathCapabilities.LEASE_RECOVERABLE;
+
+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.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public abstract class AbstractContractLeaseRecoveryTest extends
+ AbstractFSContractTestBase {
+
+ @Test
+ public void testLeaseRecovery() throws Throwable {
+ final Path path = methodPath();
+ final FileSystem fs = getFileSystem();
+ ContractTestUtils.touch(fs, path);
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+
+ Assertions.assertThat(leaseRecoverableFs.recoverLease(path))
+ .describedAs("Issuing lease recovery on a closed file must be
successful")
+ .isTrue();
+
+ Assertions.assertThat(leaseRecoverableFs.isFileClosed(path))
+ .describedAs("Get the isFileClose status on a closed file must be
successful")
+ .isTrue();
+ }
+
+ @Test
+ public void testLeaseRecoveryFileNotExist() throws Throwable {
+ final Path path = new Path("notExist");
+ final FileSystem fs = getFileSystem();
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+
+ Assertions.assertThatThrownBy(() -> leaseRecoverableFs.recoverLease(path))
Review Comment:
I'm going to say "use our `LambdaTestUtils.intercept`" method, especially
when calling a method which returns a value, as when intercept() raises an
assertion on non-raising of an exception, *it includes the toString() value of
the call*. this makes it a lot better to debug why things fail, especially if
you can return useful stuff.
Yes, it's something exclusive to our project, but it's been lifted straight
from scalatest.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/contract/hdfs/TestHDFSContractLeaseRecovery.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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 java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
Review Comment:
import structure
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestHDFSFileSystemContract.java:
##########
@@ -72,4 +76,16 @@ protected int getGlobalTimeout() {
public void testAppend() throws IOException {
AppendTestUtil.testAppend(fs, new Path("/testAppend/f"));
}
+
+ @Test
+ public void testFileSystemCapabilities() throws Exception {
+ final Path p = new Path("testFileSystemCapabilities");
+ final boolean leaseRecovery = fs.hasPathCapability(p, LEASE_RECOVERABLE);
+ Assertions.assertThat(leaseRecovery)
+ .describedAs("path capabilities %s=%s in %s",
+ LEASE_RECOVERABLE, leaseRecovery, fs)
+ .isTrue();
+ // we should not have enter safe mode when checking it.
+ Assertions.assertThat(fs instanceof SafeMode).isTrue();
Review Comment:
```
assertThat(fs)
.describedAs("filesystem %s", fs)
.isInstanceOf(SafeMode.class);
```
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractLeaseRecoveryTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 static org.apache.hadoop.fs.CommonPathCapabilities.LEASE_RECOVERABLE;
+
+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.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public abstract class AbstractContractLeaseRecoveryTest extends
+ AbstractFSContractTestBase {
+
+ @Test
+ public void testLeaseRecovery() throws Throwable {
+ final Path path = methodPath();
+ final FileSystem fs = getFileSystem();
+ ContractTestUtils.touch(fs, path);
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+
+ Assertions.assertThat(leaseRecoverableFs.recoverLease(path))
+ .describedAs("Issuing lease recovery on a closed file must be
successful")
+ .isTrue();
+
+ Assertions.assertThat(leaseRecoverableFs.isFileClosed(path))
+ .describedAs("Get the isFileClose status on a closed file must be
successful")
+ .isTrue();
+ }
+
+ @Test
+ public void testLeaseRecoveryFileNotExist() throws Throwable {
+ final Path path = new Path("notExist");
+ final FileSystem fs = getFileSystem();
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+
+ Assertions.assertThatThrownBy(() -> leaseRecoverableFs.recoverLease(path))
+ .describedAs("Issuing lease recovery on a non exist file must throw
exception")
+ .hasMessageContaining("File does not exist")
+ .isInstanceOf(FileNotFoundException.class);
+
+ Assertions.assertThatThrownBy(() ->leaseRecoverableFs.isFileClosed(path))
+ .describedAs("Get the isFileClose status on non exist file must throw
exception")
+ .hasMessageContaining("File does not exist")
+ .isInstanceOf(FileNotFoundException.class);
+ }
+
+ @Test
+ public void testLeaseRecoveryFileOnDirectory() throws Throwable {
+ final Path path = methodPath();
+ final FileSystem fs = getFileSystem();
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+ final Path parentDirectory = path.getParent();
+
+ Assertions.assertThatThrownBy(() ->
leaseRecoverableFs.recoverLease(parentDirectory))
+ .describedAs("Issuing lease recovery on a directory must throw
exception")
+ .hasMessageContaining("Path is not a file")
+ .isInstanceOf(FileNotFoundException.class);
+
+ Assertions.assertThatThrownBy(()
->leaseRecoverableFs.isFileClosed(parentDirectory))
+ .describedAs("Get the isFileClose status on a directory must throw
exception")
+ .hasMessageContaining("Path is not a file")
+ .isInstanceOf(FileNotFoundException.class);
+ }
+
+ private LeaseRecoverable verifyAndGetLeaseRecoverableInstance(FileSystem fs,
Path path)
+ throws IOException {
+ Assertions.assertThat(fs.hasPathCapability(path, LEASE_RECOVERABLE))
+ .describedAs("path capability %s of %s",
+ LEASE_RECOVERABLE, path)
+ .isTrue();
+ Assertions.assertThat(fs instanceof LeaseRecoverable)
Review Comment:
```
assertThat(fs)
.describedAs("filesystem %s", fs)
.isInstanceOf(LeaseRecoverable.class);
```
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractLeaseRecoveryTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 static org.apache.hadoop.fs.CommonPathCapabilities.LEASE_RECOVERABLE;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import org.apache.hadoop.fs.FileSystem;
Review Comment:
split the import blocks
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractSafeModeTest.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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 org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.SafeMode;
+import org.apache.hadoop.fs.SafeModeAction;
+import org.assertj.core.api.Assertions;
Review Comment:
split the import blocks
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractLeaseRecoveryTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 static org.apache.hadoop.fs.CommonPathCapabilities.LEASE_RECOVERABLE;
+
+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.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public abstract class AbstractContractLeaseRecoveryTest extends
+ AbstractFSContractTestBase {
+
+ @Test
+ public void testLeaseRecovery() throws Throwable {
+ final Path path = methodPath();
+ final FileSystem fs = getFileSystem();
+ ContractTestUtils.touch(fs, path);
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+
+ Assertions.assertThat(leaseRecoverableFs.recoverLease(path))
+ .describedAs("Issuing lease recovery on a closed file must be
successful")
+ .isTrue();
+
+ Assertions.assertThat(leaseRecoverableFs.isFileClosed(path))
+ .describedAs("Get the isFileClose status on a closed file must be
successful")
+ .isTrue();
+ }
+
+ @Test
+ public void testLeaseRecoveryFileNotExist() throws Throwable {
+ final Path path = new Path("notExist");
+ final FileSystem fs = getFileSystem();
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+
+ Assertions.assertThatThrownBy(() -> leaseRecoverableFs.recoverLease(path))
+ .describedAs("Issuing lease recovery on a non exist file must throw
exception")
+ .hasMessageContaining("File does not exist")
+ .isInstanceOf(FileNotFoundException.class);
+
+ Assertions.assertThatThrownBy(() ->leaseRecoverableFs.isFileClosed(path))
+ .describedAs("Get the isFileClose status on non exist file must throw
exception")
+ .hasMessageContaining("File does not exist")
+ .isInstanceOf(FileNotFoundException.class);
+ }
+
+ @Test
+ public void testLeaseRecoveryFileOnDirectory() throws Throwable {
+ final Path path = methodPath();
+ final FileSystem fs = getFileSystem();
+ LeaseRecoverable leaseRecoverableFs =
verifyAndGetLeaseRecoverableInstance(fs, path);
+ final Path parentDirectory = path.getParent();
+
+ Assertions.assertThatThrownBy(() ->
leaseRecoverableFs.recoverLease(parentDirectory))
+ .describedAs("Issuing lease recovery on a directory must throw
exception")
+ .hasMessageContaining("Path is not a file")
+ .isInstanceOf(FileNotFoundException.class);
+
+ Assertions.assertThatThrownBy(()
->leaseRecoverableFs.isFileClosed(parentDirectory))
+ .describedAs("Get the isFileClose status on a directory must throw
exception")
+ .hasMessageContaining("Path is not a file")
+ .isInstanceOf(FileNotFoundException.class);
+ }
+
+ private LeaseRecoverable verifyAndGetLeaseRecoverableInstance(FileSystem fs,
Path path)
+ throws IOException {
+ Assertions.assertThat(fs.hasPathCapability(path, LEASE_RECOVERABLE))
Review Comment:
FYI, there's a `assertHasPathCapabilities` method in ContractTestUtils; no
need to change it here as it is no more informative, but something to know in
future
##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:
##########
@@ -1633,6 +1636,53 @@ public DatanodeInfo[] getDataNodeStats(final
DatanodeReportType type)
* @see org.apache.hadoop.hdfs.protocol.ClientProtocol#setSafeMode(
* HdfsConstants.SafeModeAction,boolean)
*/
+ @Override
+ public boolean setSafeMode(SafeModeAction action)
+ throws IOException {
+ return setSafeMode(action, false);
+ }
+
+ /**
+ * Enter, leave or get safe mode.
+ *
+ * @param action
+ * One of SafeModeAction.ENTER, SafeModeAction.LEAVE and
+ * SafeModeAction.GET
+ * @param isChecked
+ * If true check only for Active NNs status, else check first NN's
+ * status
+ */
+ @Override
+ public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+ throws IOException {
+ return dfs.setSafeMode(convertToClientProtocolSafeModeAction(action),
isChecked);
+ }
+
+ private static HdfsConstants.SafeModeAction
convertToClientProtocolSafeModeAction(
Review Comment:
javadocs?
##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:
##########
@@ -1633,6 +1636,53 @@ public DatanodeInfo[] getDataNodeStats(final
DatanodeReportType type)
* @see org.apache.hadoop.hdfs.protocol.ClientProtocol#setSafeMode(
* HdfsConstants.SafeModeAction,boolean)
*/
+ @Override
+ public boolean setSafeMode(SafeModeAction action)
+ throws IOException {
+ return setSafeMode(action, false);
+ }
+
+ /**
+ * Enter, leave or get safe mode.
+ *
+ * @param action
+ * One of SafeModeAction.ENTER, SafeModeAction.LEAVE and
+ * SafeModeAction.GET
+ * @param isChecked
+ * If true check only for Active NNs status, else check first NN's
+ * status
+ */
+ @Override
+ public boolean setSafeMode(SafeModeAction action, boolean isChecked)
+ throws IOException {
+ return dfs.setSafeMode(convertToClientProtocolSafeModeAction(action),
isChecked);
Review Comment:
how about translating to the hdfs action and calling it on *this* class.
This ensures that existing subclasses, especially ViewDistributedFileSystem get
the request forwarded. Without it, `ViewDistributedFileSystem` is broken
--
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]
