[
https://issues.apache.org/jira/browse/HADOOP-18671?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17715812#comment-17715812
]
ASF GitHub Bot commented on HADOOP-18671:
-----------------------------------------
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
> 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: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
