Details
-
Bug
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
2.6.0
-
None
-
None
Description
Currently RoundRobinVolumeChoosingPolicy use a shared index to choose volumes in HDFS with heterogeneous storages, this leads to non-RR choosing mode for certain type of storage.
Besides, it uses a shared lock for synchronization which limits the concurrency of volume choosing process. Volume choosing threads that operating on different storage types should be able to run concurrently.
Attachments
Attachments
- HDFS-9608.01.patch
- 6 kB
- Wei Zhou
- HDFS-9608.02.patch
- 7 kB
- Wei Zhou
- HDFS-9608.03.patch
- 5 kB
- Wei Zhou
- HDFS-9608.04.patch
- 7 kB
- Wei Zhou
- HDFS-9608.05.patch
- 7 kB
- Wei Zhou
- HDFS-9608.06.patch
- 10 kB
- Wei Zhou
- HDFS-9608.07.patch
- 10 kB
- Wei Zhou
Activity
Thanks Wei for the work. The patch makes sense to me.
- In addition to the work as described, it also uses fine-grained locks for the supported storage types when choosing volumes. It's good, you may update the issue description to reflect this as well.
- The change won't impact and is compatible with clusters of no more than one storage types, but will impact clusters that deploy HSM. However, the behavior would be desired and can provide better performance.
In the test codes:
1. Better to provide both positive and negative tests as existing codes do;
2. + // with volume and block sizes in exception message, what did it mean?
3. Block size of 0 is used to choose a volume. Would you add test case that uses a practical one?
Thanks Kai for the helpful suggestions! Modifications made to the previous patch accordingly. Sorry for Item 2, i just forgot to delete it. Thanks!
Patch looks good to me, nice find! For any watchers, the issue IIUC is that the RR chooser has a single shared iterator for all storage types.
Out of curiosity, did you notice any perf issues from the shared lock?
Some nits:
- Please add some short javadoc to curVolumes and syncLocks to explain how the indexing and locking works
- Some of the lines are longer than 80chars, but we'll catch that and other stuff in precommit.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 7m 33s | trunk passed |
+1 | compile | 0m 41s | trunk passed with JDK v1.8.0_66 |
+1 | compile | 0m 41s | trunk passed with JDK v1.7.0_91 |
+1 | checkstyle | 0m 16s | trunk passed |
+1 | mvnsite | 0m 53s | trunk passed |
+1 | mvneclipse | 0m 13s | trunk passed |
+1 | findbugs | 1m 55s | trunk passed |
+1 | javadoc | 1m 4s | trunk passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 46s | trunk passed with JDK v1.7.0_91 |
+1 | mvninstall | 0m 43s | the patch passed |
+1 | compile | 0m 35s | the patch passed with JDK v1.8.0_66 |
+1 | javac | 0m 35s | the patch passed |
+1 | compile | 0m 39s | the patch passed with JDK v1.7.0_91 |
+1 | javac | 0m 39s | the patch passed |
+1 | checkstyle | 0m 15s | the patch passed |
+1 | mvnsite | 0m 49s | the patch passed |
+1 | mvneclipse | 0m 11s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
+1 | findbugs | 1m 58s | the patch passed |
+1 | javadoc | 0m 59s | the patch passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 44s | the patch passed with JDK v1.7.0_91 |
-1 | unit | 52m 3s | hadoop-hdfs in the patch failed with JDK v1.8.0_66. |
-1 | unit | 49m 31s | hadoop-hdfs in the patch failed with JDK v1.7.0_91. |
+1 | asflicense | 0m 21s | Patch does not generate ASF License warnings. |
126m 57s |
Reason | Tests |
---|---|
JDK v1.8.0_66 Failed junit tests | hadoop.hdfs.server.namenode.TestNNThroughputBenchmark |
hadoop.hdfs.server.datanode.fsdataset.TestRoundRobinVolumeChoosingPolicy | |
hadoop.hdfs.server.datanode.TestDataNodeMetrics | |
hadoop.hdfs.server.datanode.fsdataset.TestAvailableSpaceVolumeChoosingPolicy | |
JDK v1.7.0_91 Failed junit tests | hadoop.hdfs.server.datanode.fsdataset.TestRoundRobinVolumeChoosingPolicy |
hadoop.hdfs.server.datanode.fsdataset.TestAvailableSpaceVolumeChoosingPolicy | |
hadoop.hdfs.TestDFSStripedOutputStream | |
hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped |
This message was automatically generated.
Thanks for reviewing the patch. The patch is modified as suggested, thanks!
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
-1 | patch | 0m 7s | |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12782535/HDFS-9608.03.patch |
JIRA Issue | |
Powered by | Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org |
Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/14132/console |
This message was automatically generated.
Patch doesn't apply for me either, also a small nit when you rebase, javadoc is associated with the immediately following member, so recommend either splitting the syncLocks doc out to be above the syncLocks variable, or just change it to a plain block comment instead of javadoc e.g.:
/*
Unit test failures also look related.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 7m 47s | trunk passed |
+1 | compile | 0m 51s | trunk passed with JDK v1.8.0_66 |
+1 | compile | 0m 43s | trunk passed with JDK v1.7.0_91 |
+1 | checkstyle | 0m 16s | trunk passed |
+1 | mvnsite | 0m 53s | trunk passed |
+1 | mvneclipse | 0m 13s | trunk passed |
+1 | findbugs | 1m 56s | trunk passed |
+1 | javadoc | 1m 9s | trunk passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 48s | trunk passed with JDK v1.7.0_91 |
+1 | mvninstall | 0m 46s | the patch passed |
+1 | compile | 0m 47s | the patch passed with JDK v1.8.0_66 |
+1 | javac | 0m 47s | the patch passed |
+1 | compile | 0m 39s | the patch passed with JDK v1.7.0_91 |
+1 | javac | 0m 39s | the patch passed |
+1 | checkstyle | 0m 15s | the patch passed |
+1 | mvnsite | 0m 51s | the patch passed |
+1 | mvneclipse | 0m 10s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
+1 | findbugs | 2m 8s | the patch passed |
+1 | javadoc | 1m 13s | the patch passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 47s | the patch passed with JDK v1.7.0_91 |
-1 | unit | 63m 38s | hadoop-hdfs in the patch failed with JDK v1.8.0_66. |
-1 | unit | 56m 59s | hadoop-hdfs in the patch failed with JDK v1.7.0_91. |
+1 | asflicense | 0m 18s | Patch does not generate ASF License warnings. |
147m 36s |
Reason | Tests |
---|---|
JDK v1.8.0_66 Failed junit tests | hadoop.hdfs.server.datanode.fsdataset.TestAvailableSpaceVolumeChoosingPolicy |
hadoop.hdfs.server.namenode.TestNNThroughputBenchmark | |
hadoop.hdfs.TestCrcCorruption | |
hadoop.hdfs.server.datanode.fsdataset.TestRoundRobinVolumeChoosingPolicy | |
JDK v1.7.0_91 Failed junit tests | hadoop.hdfs.server.datanode.fsdataset.TestAvailableSpaceVolumeChoosingPolicy |
hadoop.hdfs.server.namenode.TestNNThroughputBenchmark | |
hadoop.hdfs.TestDFSClientRetries | |
hadoop.hdfs.server.datanode.fsdataset.TestRoundRobinVolumeChoosingPolicy | |
hadoop.hdfs.server.datanode.TestBlockScanner |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 8m 36s | trunk passed |
+1 | compile | 0m 56s | trunk passed with JDK v1.8.0_66 |
+1 | compile | 0m 47s | trunk passed with JDK v1.7.0_91 |
+1 | checkstyle | 0m 18s | trunk passed |
+1 | mvnsite | 1m 4s | trunk passed |
+1 | mvneclipse | 0m 15s | trunk passed |
+1 | findbugs | 2m 2s | trunk passed |
+1 | javadoc | 1m 8s | trunk passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 49s | trunk passed with JDK v1.7.0_91 |
+1 | mvninstall | 0m 49s | the patch passed |
+1 | compile | 0m 42s | the patch passed with JDK v1.8.0_66 |
+1 | javac | 0m 42s | the patch passed |
+1 | compile | 0m 41s | the patch passed with JDK v1.7.0_91 |
+1 | javac | 0m 41s | the patch passed |
+1 | checkstyle | 0m 15s | the patch passed |
+1 | mvnsite | 0m 53s | the patch passed |
+1 | mvneclipse | 0m 13s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
+1 | findbugs | 2m 6s | the patch passed |
+1 | javadoc | 1m 7s | the patch passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 45s | the patch passed with JDK v1.7.0_91 |
-1 | unit | 68m 24s | hadoop-hdfs in the patch failed with JDK v1.8.0_66. |
-1 | unit | 68m 28s | hadoop-hdfs in the patch failed with JDK v1.7.0_91. |
+1 | asflicense | 0m 30s | Patch does not generate ASF License warnings. |
165m 16s |
Reason | Tests |
---|---|
JDK v1.8.0_66 Failed junit tests | hadoop.hdfs.TestLeaseRecovery2 |
hadoop.hdfs.server.namenode.ha.TestRequestHedgingProxyProvider | |
hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot | |
JDK v1.7.0_91 Failed junit tests | hadoop.hdfs.shortcircuit.TestShortCircuitCache |
hadoop.hdfs.server.namenode.TestNamenodeCapacityReport | |
hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot |
This message was automatically generated.
Changes looks good.
I have some comments,
1. To reduce the number of lines changed, Instead of moving entire while loop inside synchronized block, just extract that part to a method and call that method from synchronized block.
As below.
@Override public synchronized V chooseVolume(final List<V> volumes, long blockSize) @@ -40,13 +51,24 @@ public synchronized V chooseVolume(final List<V> volumes, long blockSize) if(volumes.size() < 1) { throw new DiskOutOfSpaceException("No more available volumes"); } - + + // As all the items in volumes are with the same storage type, + // so only need to get the storage type index of the first item in volumes + StorageType storageType = volumes.get(0).getStorageType(); + int index = storageType != null ? + storageType.ordinal() : StorageType.DEFAULT.ordinal(); + synchronized (syncLocks[index]) { + return chooseVolume(index, volumes, blockSize); + } + } + + private V chooseVolume(final int curVolumesIndex, final List<V> volumes, + long blockSize) throws IOException { // since volumes could've been removed because of the failure // make sure we are not out of bounds - if(curVolume >= volumes.size()) { - curVolume = 0; - } - + int curVolume = curVolumes[curVolumesIndex] < volumes.size() + ? curVolumes[curVolumesIndex] : 0; + int startVolume = curVolume; long maxAvailable = 0; @@ -55,6 +77,7 @@ public synchronized V chooseVolume(final List<V> volumes, long blockSize) curVolume = (curVolume + 1) % volumes.size(); long availableVolumeSize = volume.getAvailable(); if (availableVolumeSize > blockSize) { + curVolumes[curVolumesIndex] = curVolume; return volume; }
2. Is this idea ( separate lock for each storage type) applicable for AvailableSpaceVolumeChoosingPolicy as well?
Thanks Vinayakumar for the suggestions! Code refine and add fine-grained locks for AvailableSpaceVolumeChoosingPolicy.
Thanks for your work! Code refined as your suggestion.
For question 2, I planed to open another JIRA because RR policy is used by AvailableSpaceVolumeChoosingPolicy. It is now included in this patch, thanks!
+1 for the latest update. thanks zhouwei
andrew.wang, do you have any inputs here?
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 11m 0s | trunk passed |
+1 | compile | 1m 14s | trunk passed with JDK v1.8.0_66 |
+1 | compile | 1m 6s | trunk passed with JDK v1.7.0_91 |
+1 | checkstyle | 0m 26s | trunk passed |
+1 | mvnsite | 1m 26s | trunk passed |
+1 | mvneclipse | 0m 22s | trunk passed |
+1 | findbugs | 2m 55s | trunk passed |
+1 | javadoc | 2m 0s | trunk passed with JDK v1.8.0_66 |
+1 | javadoc | 3m 21s | trunk passed with JDK v1.7.0_91 |
+1 | mvninstall | 1m 15s | the patch passed |
+1 | compile | 1m 35s | the patch passed with JDK v1.8.0_66 |
+1 | javac | 1m 35s | the patch passed |
+1 | compile | 1m 6s | the patch passed with JDK v1.7.0_91 |
+1 | javac | 1m 6s | the patch passed |
+1 | checkstyle | 0m 25s | the patch passed |
+1 | mvnsite | 1m 22s | the patch passed |
+1 | mvneclipse | 0m 18s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
-1 | findbugs | 3m 22s | hadoop-hdfs-project/hadoop-hdfs generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
+1 | javadoc | 2m 7s | the patch passed with JDK v1.8.0_66 |
+1 | javadoc | 3m 6s | the patch passed with JDK v1.7.0_91 |
-1 | unit | 86m 20s | hadoop-hdfs in the patch failed with JDK v1.8.0_66. |
-1 | unit | 75m 40s | hadoop-hdfs in the patch failed with JDK v1.7.0_91. |
+1 | asflicense | 0m 26s | Patch does not generate ASF License warnings. |
204m 58s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-hdfs-project/hadoop-hdfs |
Inconsistent synchronization of org.apache.hadoop.hdfs.server.datanode.fsdataset.AvailableSpaceVolumeChoosingPolicy.balancedPreferencePercent; locked 57% of time Unsynchronized access at AvailableSpaceVolumeChoosingPolicy.java:57% of time Unsynchronized access at AvailableSpaceVolumeChoosingPolicy.java:[line 159] | |
JDK v1.8.0_66 Failed junit tests | hadoop.tracing.TestTracing |
hadoop.hdfs.server.namenode.ha.TestDFSUpgradeWithHA | |
hadoop.hdfs.server.datanode.TestBlockScanner | |
hadoop.hdfs.server.datanode.TestDirectoryScanner | |
hadoop.hdfs.server.blockmanagement.TestReplicationPolicy | |
hadoop.hdfs.server.namenode.TestNNThroughputBenchmark | |
hadoop.hdfs.security.TestDelegationTokenForProxyUser | |
JDK v1.8.0_66 Timed out junit tests | org.apache.hadoop.cli.TestHDFSCLI |
JDK v1.7.0_91 Failed junit tests | hadoop.hdfs.server.blockmanagement.TestBlockManagerSafeMode |
hadoop.hdfs.server.namenode.ha.TestRequestHedgingProxyProvider | |
hadoop.fs.viewfs.TestViewFileSystemAtHdfsRoot | |
hadoop.hdfs.server.datanode.TestBlockScanner | |
hadoop.hdfs.server.mover.TestStorageMover | |
hadoop.hdfs.server.datanode.TestDirectoryScanner | |
hadoop.fs.TestSymlinkHdfsFileContext | |
hadoop.hdfs.security.TestDelegationTokenForProxyUser |
This message was automatically generated.
Inconsistent synchronization of org.apache.hadoop.hdfs.server.datanode.fsdataset.AvailableSpaceVolumeChoosingPolicy.balancedPreferencePercent; locked 57% of time Unsynchronized access at AvailableSpaceVolumeChoosingPolicy.java:57% of time Unsynchronized access at AvailableSpaceVolumeChoosingPolicy.java:[line 159]
didnt see that coming.
IMO, its safe to remove synchronization on getConf() and setConf() in AvailableSpaceVolumeChoosingPolicy. as its nowhere else called
Agree with Vinay's analysis, I didn't see any other usages of getConf/setConf either.
+1 pending this and another Jenkins run.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 7m 37s | trunk passed |
+1 | compile | 0m 41s | trunk passed with JDK v1.8.0_66 |
+1 | compile | 0m 41s | trunk passed with JDK v1.7.0_91 |
+1 | checkstyle | 0m 15s | trunk passed |
+1 | mvnsite | 0m 51s | trunk passed |
+1 | mvneclipse | 0m 13s | trunk passed |
+1 | findbugs | 1m 55s | trunk passed |
+1 | javadoc | 1m 5s | trunk passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 46s | trunk passed with JDK v1.7.0_91 |
+1 | mvninstall | 0m 45s | the patch passed |
+1 | compile | 0m 35s | the patch passed with JDK v1.8.0_66 |
+1 | javac | 0m 35s | the patch passed |
+1 | compile | 0m 37s | the patch passed with JDK v1.7.0_91 |
+1 | javac | 0m 37s | the patch passed |
+1 | checkstyle | 0m 15s | the patch passed |
+1 | mvnsite | 0m 51s | the patch passed |
+1 | mvneclipse | 0m 11s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
+1 | findbugs | 2m 9s | the patch passed |
+1 | javadoc | 1m 3s | the patch passed with JDK v1.8.0_66 |
+1 | javadoc | 1m 45s | the patch passed with JDK v1.7.0_91 |
-1 | unit | 52m 17s | hadoop-hdfs in the patch failed with JDK v1.8.0_66. |
+1 | unit | 49m 28s | hadoop-hdfs in the patch passed with JDK v1.7.0_91. |
+1 | asflicense | 0m 22s | Patch does not generate ASF License warnings. |
127m 42s |
Reason | Tests |
---|---|
JDK v1.8.0_66 Failed junit tests | hadoop.hdfs.server.namenode.TestNNThroughputBenchmark |
hadoop.hdfs.server.datanode.TestBlockScanner | |
hadoop.hdfs.TestDFSUpgradeFromImage |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:0ca8df7 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12783494/HDFS-9608.07.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 1ae6bb734950 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 1708a4c |
Default Java | 1.7.0_91 |
Multi-JDK versions | /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91 |
findbugs | v3.0.0 |
unit | https://builds.apache.org/job/PreCommit-HDFS-Build/14180/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt |
unit test logs | https://builds.apache.org/job/PreCommit-HDFS-Build/14180/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt |
JDK v1.7.0_91 Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/14180/testReport/ |
modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
Max memory used | 76MB |
Powered by | Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org |
Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/14180/console |
This message was automatically generated.
Committed to trunk and branch-2, thanks Wei for find and fix, Vinay for reviewing!
FAILURE: Integrated in Hadoop-trunk-Commit #9314 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9314/)
HDFS-9608. Disk IO imbalance in HDFS with heterogeneous storages. (wang: rev 3a23dc683c058d3a5262ae9dca2d1c8c588a6a3e)
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/TestRoundRobinVolumeChoosingPolicy.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/RoundRobinVolumeChoosingPolicy.java
- hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/AvailableSpaceVolumeChoosingPolicy.java
- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
Should this go to branch-2.8 (possibly 2.7) as well?
IMO, it should. Any other opinions?
Hi,
I'm on vacation from 2/8/2018 and back on 2/22/2018. My email response may be delayed. Thanks!
Wish you happly Chinese New Year!
Best wishes
Wei
This patch insure choosing volumes in RR mode for each storage type. Besides, it uses storage-type-level lock to allow concurrent choosing of different storage types. Thanks!