GitHub user arp7 opened a pull request:

https://github.com/apache/hadoop/pull/155

HDFS-11119. Support for parallel checking of StorageLocations on DataNode startup

Introduce a StorageLocationChecker class that can parallelize checking StorageLocations. It also detects stalled checks and flags such volumes as failed. The DataNode will use this class in the next Jira.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/arp7/hadoop HDFS-11119

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/hadoop/pull/155.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #155

commit 3561232c37de5eddde6185cec60933e0c687bbed
Author: Arpit Agarwal <arp@apache.org>
Date: 2016-11-08T22:41:59Z

HDFS-11119. Support for parallel checking of StorageLocations on DataNode startup.

Arpit Agarwal Thanks for the patch. Some minor comments

The way time is specified is little confusing. Just wanted to make sure that my understanding is correct.

public static final String DFS_DATANODE_DISK_CHECK_TIMEOUT_DEFAULT ="10m";
public static final String DFS_DATANODE_DISK_CHECK_MIN_GAP_DEFAULT ="15m";

This is milliseconds right ? but if that is hardcoded in code, then why take an "m" if all of these are time units in milliseconds.

When I first read code I read 15m as 15 minutes and later realized that it is 15 milliseconds. I think we should pick a larger min gap and also if possible specify that the time unit is not flexible.

In hdfs-default.xml also can we specify that the time unit is in milliseconds. It is obvious from code, but I am worried some user might try to specify something like H.

StorageLocationsChecker.java#check

    if (goodLocations.size() == 0) {
      throw new IOException("All directories in "
          + DFS_DATANODE_DATA_DIR_KEY + " are invalid: "
          + failedLocations);
    }

Should we Log.error this case too ?

StorageLocationsChecker.java
Executors.newCachedThreadPool()
Can we take a ThreadFactory so we can set the name of threads in this pool ? Plus, Are these threads daemons ?

nit : I am presuming the change in Datanode startup is coming in a later patch ?

Thank you for the review Anu Engineer. I have updated the PR with a commit to address your comments. A few responses below:

1. This is milliseconds right ? but if that is hardcoded in code, then why take an "m" if all of these are time units in milliseconds.

   In hdfs-default.xml also can we specify that the time unit is in milliseconds.

   We use Configuration#getTimeDuration which supports suffixes for common time units (m implies minutes). However you are right that this is not at all obvious. I updated the descriptions in hdfs-default.xml with pointers to the list of supported suffixes.

2. Should we Log.error this case too ?

   Good point. This error is logged as a fatal exception by the DataNode so we shouldn't need a separate message. I made the behavior consistent for both failure cases. Let me know what you think.

3. Can we take a ThreadFactory so we can set the name of threads in this pool ? Plus, Are these threads daemons ?

   Good catch. Fixed both points.

4. nit : I am presuming the change in Datanode startup is coming in a later patch ?

   Correct.

Arpit Agarwal Thank you for updating the patch. +1, pending jenkins.

-1 overall

This message was automatically generated.

Updated the pull request with 1 more commit for checkstyle fixes.
https://github.com/apache/hadoop/pull/155/commits/8b0e911a387fe63a9ae021156220f5c720b3f2db

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Github user anuengineer commented on the issue:

https://github.com/apache/hadoop/pull/155

+1, LGTM

Github user asfgit closed the pull request at:

https://github.com/apache/hadoop/pull/155

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10826 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10826/)
HDFS-11119. Support for parallel checking of StorageLocations on (arp: rev 3d267177776547ceb32c5b9ed04cd9ec05b3421a)

• (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/VolumeCheckResult.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
• (add) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/checker/TestStorageLocationChecker.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
• (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/StorageLocationChecker.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/StorageLocation.java

Thank you for the reviews Anu Engineer. I've committed this to trunk.

Cherry-picked to branch-2.