Hi Lei (Eddy) Xu. Another potential problem that I've noticed in the DataNode reconfiguration code is that it never recalculates FsDatasetImpl#validVolsRequired. This is a final variable calculated as (# volumes configured) - (# volume failures tolerated):

    this.validVolsRequired = volsConfigured - volFailuresTolerated;

If this variable is not updated for DataNode reconfigurations, then it could lead to some unexpected situations. For example:

1. DataNode starts running with 6 volumes (all healthy) and dfs.datanode.failed.volumes.tolerated set to 2.
2. FsDatasetImpl#validVolsRequired is set to 6 - 2 = 4.
3. DataNode is reconfigured to run with 8 volumes (all still healthy).
4. Now 3 volumes fail. The admin would expect the DataNode to abort, but there are 8 - 3 = 5 good volumes left, and FsDatasetImpl#validVolsRequired is still 4, so FsDatasetImpl#hasEnoughResource returns true.

Is this something that makes sense for you to address as part of the patch you're working on now, or would you prefer I file a separate jira to track this? Thanks!

Chris Nauroth Would you mind to file a separate JIRA and assign to me? Thanks!

Thank you, Eddy. I filed HDFS-7833.

This patch releases the lock on StorageDirectory in FsDatasetImpl#addVolume if there is a failure after the StorageDirectory being successfully built:

    if (!exceptions.isEmpty()) {
      sd.unlock();
      throw MultipleIOException.createIOException(exceptions);
    }

Also added a test to inject a IOE after building StorageDirectory.

At the last, it moves TestDataNodeHotSwapVolumes#assertFileLocksReleased() to FsDatasetTestUtil.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12702015/HDFS-7830.000.patch
against trunk revision c5eac9c.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 3 new or modified test files.

-1 javac. The applied patch generated 1156 javac compiler warnings (more than the trunk's current 1155 warnings).

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.TestReplaceDatanodeOnFailure

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9703//testReport/
Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9703//artifact/patchprocess/diffJavacWarnings.txt
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9703//console

This message is automatically generated.

108	    } finally {
109	      if (lock != null) {
110	        try {
111	          lock.release();
112	        } catch (IOException e) {
113	          FsDatasetImpl.LOG.warn(String.format("I/O error releasing file lock %s.",
114	              lockFile.getAbsolutePath()), e);
115	        }

We shouldn't swallow the exception here in the unit tests. If the lock file can't be released, the unit test should fail. So we should not catch the exception (or if we do, we should rethrow it).

420	    if (!exceptions.isEmpty()) {
421	      sd.unlock();
422	      throw MultipleIOException.createIOException(exceptions);

In the non-unit-test case, we do need to catch the exception and prevent it from propagating, since then we won't see any other exceptions.

Colin P. McCabe Thanks for the reviews. I updated the patch to address your comments.

We shouldn't swallow the exception here in the unit tests.

Fixed.

In the non-unit-test case, we do need to catch the exception and prevent it from propagating, since then we won't see any other exceptions.

The IOException is captured from:

try {
  fsVolume.addBlockPool(bpid, this.conf);
  fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);
} catch (IOException e) {
    LOG.warn("Caught exception when adding " + fsVolume +
            ". Will throw later.", e);
     exceptions.add(e);
}

And these {{IOE}}s are used in {{DataNode#refreshVolumes}} to build error messages. It is still needed to be thrown. What do you think?

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12703489/HDFS-7830.001.patch
against trunk revision 3241fc2.

-1 patch. Trunk compilation may be broken.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9798//console

This message is automatically generated.

The failure is due to errors on jenkins server.

420	    if (!exceptions.isEmpty()) {
421	      sd.unlock();
422	      throw MultipleIOException.createIOException(exceptions);
423	    }

The point that I was making is that if line 421 throws, we won't see the exceptions on line 422. We should gather up this exception into our list of exceptions like the rest.

Sorry. I misunderstood your comments Colin P. McCabe.

Updated this patch to address the comments correctly.

Thanks, Lei (Eddy) Xu. +1 pending jenkins.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12703521/HDFS-7830.002.patch
against trunk revision d6e05c5.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 3 new or modified test files.

-1 javac. The applied patch generated 1156 javac compiler warnings (more than the trunk's current 1155 warnings).

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9804//testReport/
Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9804//artifact/patchprocess/diffJavacWarnings.txt
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9804//console

This message is automatically generated.

Colin P. McCabe Thanks a lot for your review and inputs.

I updated the patch to address the javac warning.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12703695/HDFS-7830.003.patch
against trunk revision 20b8ee1.

-1 patch. Trunk compilation may be broken.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9819//console

This message is automatically generated.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12703695/HDFS-7830.003.patch
against trunk revision a380643.

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9820//console

This message is automatically generated.

Sorry. I did not rebase to trunk last time. I created a new patch after rebasing.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12703742/HDFS-7830.004.patch
against trunk revision aa92b76.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 3 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA
org.apache.hadoop.hdfs.TestAppendSnapshotTruncate
org.apache.hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot
org.apache.hadoop.hdfs.server.balancer.TestBalancer

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9823//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9823//console

This message is automatically generated.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12703742/HDFS-7830.004.patch
against trunk revision aa92b76.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 3 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9824//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9824//console

This message is automatically generated.

+1. Thanks, Lei (Eddy) Xu.

FAILURE: Integrated in Hadoop-trunk-Commit #7302 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7302/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #129 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/129/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java

FAILURE: Integrated in Hadoop-Yarn-trunk #863 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/863/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #2061 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2061/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #120 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/120/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #129 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/129/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2079 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2079/)
HDFS-7830. DataNode does not release the volume lock when adding a volume fails. (Lei Xu via Colin P. McCabe) (cmccabe: rev 5c1036d598051cf6af595740f1ab82092b0b6554)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java

Committed to branch-2.7 also. Hi Colin P. McCabe, if you didn't commit this to branch-2.7 intentionally, please feel free to revert it and bump up the fix version.

Sangjin Lee backported this to 2.6.1, after fixing minor merge issues. I just pushed the commit to 2.6.1 after running compilation and TestDataNodeHotSwapVolumes, TestFsDatasetImpl which changed in the patch.