Details
-
Type:
Improvement
-
Status: Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 3.0.0-alpha1
-
Fix Version/s: 3.0.0-alpha4
-
Component/s: shell
-
Labels:
-
Target Version/s:
-
Hadoop Flags:Reviewed
Description
Quoting my comments in HDFS-10974:
Looking at code again, I think we can improve the handling of EC files better:
- In SetReplication#processPath, can we check if the file is EC before invoking setReplication() on it?
- -setrep also has a command switch -w to wait for the file to be replicated. Can we ignore this if the file is erasure coded?
Also, in fact, -setrep only make sense for HDFS file system...
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 23s | 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. |
| 0 | mvndep | 1m 51s | Maven dependency ordering for branch |
| +1 | mvninstall | 12m 44s | trunk passed |
| +1 | compile | 16m 14s | trunk passed |
| +1 | checkstyle | 1m 57s | trunk passed |
| +1 | mvnsite | 2m 21s | trunk passed |
| +1 | mvneclipse | 0m 34s | trunk passed |
| +1 | findbugs | 3m 49s | trunk passed |
| +1 | javadoc | 1m 40s | trunk passed |
| 0 | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 | mvninstall | 1m 46s | the patch passed |
| +1 | compile | 16m 17s | the patch passed |
| +1 | javac | 16m 17s | the patch passed |
| +1 | checkstyle | 2m 1s | root: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 | mvnsite | 2m 18s | the patch passed |
| +1 | mvneclipse | 0m 35s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 3m 55s | the patch passed |
| +1 | javadoc | 1m 35s | the patch passed |
| -1 | unit | 7m 47s | hadoop-common in the patch failed. |
| +1 | unit | 63m 39s | hadoop-hdfs in the patch passed. |
| +1 | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
| 143m 50s |
| Reason | Tests |
|---|---|
| Failed junit tests | hadoop.security.TestKDiag |
| Subsystem | Report/Notes |
|---|---|
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | |
| JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12861422/HDFS-11598.001.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
| uname | Linux 61d4995d9c0f 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 28cdc5a |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
| unit | https://builds.apache.org/job/PreCommit-HDFS-Build/18932/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/18932/testReport/ |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . |
| Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/18932/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Hello Yiqun Lin thanks for your quick patch!
This is probably my oversight initially....
If we do not add an erasure coded file into waitList, we do not need to check if a file is erasure coded in waitForReplication(), correct?
For the error message "Do not set replication for: ", it reads more naturally to use past tense "Did not set replication for:"
Also in the test, would you mind to add an additional assert to make sure the replication factor of the EC file is still 1?
Thanks Wei-Chiu Chuang for the review!
If we do not add an erasure coded file into waitList, we do not need to check if a file is erasure coded in waitForReplication(), correct?
Yes, you are right. The other two comments make sense to me.
Attach the new patch.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 20s | 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. |
| 0 | mvndep | 2m 1s | Maven dependency ordering for branch |
| +1 | mvninstall | 13m 5s | trunk passed |
| +1 | compile | 14m 55s | trunk passed |
| +1 | checkstyle | 1m 54s | trunk passed |
| +1 | mvnsite | 2m 3s | trunk passed |
| +1 | mvneclipse | 0m 39s | trunk passed |
| +1 | findbugs | 3m 16s | trunk passed |
| +1 | javadoc | 1m 35s | trunk passed |
| 0 | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 | mvninstall | 1m 26s | the patch passed |
| +1 | compile | 13m 33s | the patch passed |
| +1 | javac | 13m 33s | the patch passed |
| +1 | checkstyle | 1m 55s | root: The patch generated 0 new + 17 unchanged - 1 fixed = 17 total (was 18) |
| +1 | mvnsite | 1m 59s | the patch passed |
| +1 | mvneclipse | 0m 38s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 3m 31s | the patch passed |
| +1 | javadoc | 1m 36s | the patch passed |
| +1 | unit | 8m 18s | hadoop-common in the patch passed. |
| -1 | unit | 64m 28s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
| 139m 23s |
| Reason | Tests |
|---|---|
| Failed junit tests | hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting |
| Subsystem | Report/Notes |
|---|---|
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | |
| JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12861698/HDFS-11598.002.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
| uname | Linux 4a52c528efa4 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 845529b |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
| unit | https://builds.apache.org/job/PreCommit-HDFS-Build/18954/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/18954/testReport/ |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . |
| Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/18954/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
+1
Thanks a lot for the patch Yiqun Lin. I committed your 002 patch to trunk.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11517 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11517/)
HDFS-11598. Improve -setrep for Erasure Coded files. Contributed by (weichiu: rev bbd68478d5743b3b2911bf3febed7daa89479e45)
- (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/SetReplication.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11591 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11591/)
HDFS-11598. Improve -setrep for Erasure Coded files. Contributed by (weichiu: rev bbd68478d5743b3b2911bf3febed7daa89479e45)
- (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/SetReplication.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java
Thanks for catching this, Wei-Chiu Chuang!
I take some time to make this improvement. It's a minor change but it should be nice to have in hdfs ec.
Attach my patch. Kindly review. Thanks.