HDFS-11182. Update DataNode to use DatasetVolumeChecker. #168

arp7 · 2016-11-29T01:31:00Z

Preliminary patch for Jenkins runs.

xiaoyuyao · 2016-12-19T21:26:01Z

...-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/DatasetVolumeChecker.java

+     *                       successful, add the volume here.
+     * @param failedVolumes set of failed volumes. If the disk check fails,
+     *                      add the volume here.
+     * @param semaphore semaphore used to trigger callback invocation.


The usage of semaphore here seems like a countUpLatch. Have you hit any problem with the existing CountDownLatch approach?

CountDownLatch#countDown returns no value so there is no easy way to detect when the count falls to zero and the callback can be invoked (it must be invoked once only). I was using an AtomicLong to detect the 0->1 transition but it had a bug.

The semaphore approach fixes it. We still need the CountDownLatch which we can use as an event. I could have used an Object mutex instead but that would have required extra code to deal with the spurious wakeup problem which CountDownLatch does not suffer from.

I think this logic can be simplified. Will post an updated patch shortly.

Thanks. The new logic looks good to me.

xiaoyuyao · 2016-12-19T21:28:20Z

...p-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeList.java

-      for(Iterator<FsVolumeImpl> i = volumeList.iterator(); i.hasNext(); ) {
-        final FsVolumeImpl fsv = i.next();
+      for(FsVolumeSpi vol : failedVolumes) {
+        FsVolumeImpl fsv = (FsVolumeImpl) vol;


Is it a safe cast from FsVolumeSpi to FsVolumeImpl? Can we add some log here in case the cast fail?

Yes. FsVolumeList is part of the fsdataset.impl package and its methods are only invoked from FsDatasetImpl so it is safe to assume that the volume is an FsVolumeImpl.

At least one existing method also makes the same assumption (see copyReplicaWithNewBlockIdAndGS).

private File[] copyReplicaWithNewBlockIdAndGS( ReplicaInfo replicaInfo, String bpid, long newBlkId, long newGS) throws IOException { String blockFileName = Block.BLOCK_FILE_PREFIX + newBlkId; FsVolumeImpl v = (FsVolumeImpl) replicaInfo.getVolume();

Thanks for the explanation. Looks good to me.

xiaoyuyao · 2016-12-19T21:35:13Z

...st/java/org/apache/hadoop/hdfs/server/datanode/checker/TestDatasetVolumeCheckerFailures.java

@@ -124,8 +127,10 @@ public void testMinGapIsEnforcedForSyncChecks() throws Exception {

  @Test(timeout=60000)
  public void testMinGapIsEnforcedForASyncChecks() throws Exception {
+    final List<FsVolumeSpi> volumes =


NIT: maybe wrap the common test prep code with a helper for testMinGapIsEnforcedForSyncChecks() and testMinGapIsEnforcedForASyncChecks().

Change-Id: Idbe301392050d004461079ac38548d1e62db493f

Change-Id: Icb1c8024e974a9fb1d26e5fdb3f9df34d33e8f31

Change-Id: I9b6fe60c955c2d911bc614be3619c89cda5e99ea

xiaoyuyao · 2016-12-20T03:19:39Z

...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

@@ -1944,6 +1935,8 @@ public void shutdown() {
      }
    }

+    volumeChecker.shutdownAndWait(1, TimeUnit.SECONDS);
+
    if (storageLocationChecker != null) {


The Datanode#storageLocationChecker is only needed during the datanode startup. We don't need to pass it as a parameter to DataNode constructor and keep it running during the lifetime of the datanode until datanode shutdown. This can be done as an optimization later.

Can we reuse the synchronize version of the DatasetVolume checker for datanode startup handling? This way, we don't need to maintain two checkers for Datanode? This can be done in as a follow up if possible.

Will address it in a follow up patch.

By the way regarding the reuse, I really wanted to do that too but it's non-trivial because the handling logic is different in both paths. It probably should have never been made different but reconciling them now is a bit of work. We can look at it in a separate Jira.

Agree. Let's do that in a follow up jira.

xiaoyuyao · 2016-12-20T03:25:36Z

...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

-      for (StorageLocation location : unhealthyLocations) {
-        sb.append(location + ";");
-      }
+      LOG.info(sb.toString());


Can we log a warn instead of info for the failed volumes that got removed?

Good point, will push an update shortly to improve the logging.

Thanks for fixing that.

xiaoyuyao · 2016-12-20T03:28:01Z

...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

+        data, (healthyVolumes, failedVolumes) -> {
+          LOG.info("checkDiskErrorAsync callback got {} failed volumes: {}",
+              failedVolumes.size(), failedVolumes);
+          lastDiskErrorCheck = Time.monotonicNow();


should this be timer.monotonicNow();

The DataNode does not maintain a timer object right now. It is only passed to DatasetVolumeChecker during construction for unit testability of that class.

xiaoyuyao · 2016-12-20T03:28:37Z

...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

+      unhealthyVolumes = volumeChecker.checkAllVolumes(data);
+      LOG.info("checkDiskError got {} failed volumes - {}",
+          unhealthyVolumes.size(), unhealthyVolumes);
+      lastDiskErrorCheck = Time.monotonicNow();


should this be timer.monotonicNow();

Same as above.

Change-Id: I9d5cb81f00ef7b0dde36be8e92887ee47a33c852

arp7 · 2016-12-20T04:25:05Z

@xiaoyuyao I pushed one more commit to improve the logging. Now we log at warn if there is a volume failure and at debug if there is no failure.

xiaoyuyao

Thanks for the update. +1 with the latest change.

aajisaka · 2019-07-26T11:29:10Z

https://jira.apache.org/jira/browse/HDFS-11182 has been fixed. Closing this as well.

Remove ApplicationRunner#getLocalRunner and clean up any usage examples. Author: Xinyu Liu <xiliu@xiliu-ld.linkedin.biz> Reviewers: Jake Maes <jmakes@apache.org> Closes apache#168 from xinyuiscool/SAMZA-1267

xiaoyuyao reviewed Dec 19, 2016

View reviewed changes

arp7 added 4 commits December 19, 2016 14:46

HDFS-11182. Update DataNode to use DatasetVolumeChecker.

e272c0c

Change-Id: Idbe301392050d004461079ac38548d1e62db493f

Fix unit tests, checkstyle and whitespace

e7b4b62

Address feedback from xyao.

7ef55e7

Change-Id: Icb1c8024e974a9fb1d26e5fdb3f9df34d33e8f31

Checkstyle fixes.

b7236f2

Change-Id: I9b6fe60c955c2d911bc614be3619c89cda5e99ea

xiaoyuyao reviewed Dec 20, 2016

View reviewed changes

Logging improvement.

9c4366f

Change-Id: I9d5cb81f00ef7b0dde36be8e92887ee47a33c852

xiaoyuyao approved these changes Dec 20, 2016

View reviewed changes

asfgit force-pushed the trunk branch from 25ac447 to 4d1fac5 Compare April 15, 2017 19:10

aajisaka closed this Jul 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDFS-11182. Update DataNode to use DatasetVolumeChecker. #168

HDFS-11182. Update DataNode to use DatasetVolumeChecker. #168

arp7 commented Nov 29, 2016

xiaoyuyao Dec 19, 2016

arp7 Dec 19, 2016

arp7 Dec 19, 2016

xiaoyuyao Dec 20, 2016

xiaoyuyao Dec 19, 2016

arp7 Dec 19, 2016

xiaoyuyao Dec 19, 2016

xiaoyuyao Dec 19, 2016

xiaoyuyao Dec 20, 2016

xiaoyuyao Dec 20, 2016 •

edited

Loading

arp7 Dec 20, 2016

arp7 Dec 20, 2016

xiaoyuyao Dec 20, 2016

xiaoyuyao Dec 20, 2016

arp7 Dec 20, 2016

xiaoyuyao Dec 20, 2016

xiaoyuyao Dec 20, 2016

arp7 Dec 20, 2016

xiaoyuyao Dec 20, 2016

arp7 Dec 20, 2016

arp7 commented Dec 20, 2016

xiaoyuyao left a comment

aajisaka commented Jul 26, 2019

HDFS-11182. Update DataNode to use DatasetVolumeChecker. #168

HDFS-11182. Update DataNode to use DatasetVolumeChecker. #168

Conversation

arp7 commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoyuyao Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arp7 commented Dec 20, 2016

xiaoyuyao left a comment

Choose a reason for hiding this comment

aajisaka commented Jul 26, 2019

xiaoyuyao Dec 20, 2016 •

edited

Loading