Andrew Wang added a comment - 13/Apr/17 01:41 Patch attached. Switched over to using EqualsBuilder and HashBuilder Added unit tests Added "id" field in ECPolicy to toString, equals, hashCode

Andrew Wang added a comment - 13/Apr/17 01:43 Wei-Chiu Chuang you noticed the ID issue over on another JIRA, mind reviewing this one?

Andrew Wang added a comment - 13/Apr/17 01:46 Few mistakes crept in, 002 patch.

Hadoop QA added a comment - 13/Apr/17 03:03 +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 12s Maven dependency ordering for branch +1 mvninstall 12m 34s trunk passed +1 compile 15m 21s trunk passed +1 checkstyle 1m 45s trunk passed +1 mvnsite 1m 48s trunk passed +1 mvneclipse 0m 42s trunk passed +1 findbugs 2m 56s trunk passed +1 javadoc 1m 21s trunk passed 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 1m 2s the patch passed +1 compile 13m 0s the patch passed +1 javac 13m 0s the patch passed -0 checkstyle 1m 44s root: The patch generated 5 new + 7 unchanged - 1 fixed = 12 total (was 8) +1 mvnsite 1m 37s the patch passed +1 mvneclipse 0m 42s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 3m 1s the patch passed +1 javadoc 1m 13s the patch passed +1 unit 7m 16s hadoop-common in the patch passed. +1 unit 1m 22s hadoop-hdfs-client in the patch passed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 69m 49s Subsystem Report/Notes Docker Image:yetus/hadoop:612578f JIRA Issue HDFS-11652 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12863187/HDFS-11652.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2f789eb4a895 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0cab572 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/19073/artifact/patchprocess/diff-checkstyle-root.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19073/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19073/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Wei-Chiu Chuang added a comment - 13/Apr/17 12:24 Hi Andrew, thanks for the patch. I took a quick look and found one nit: return new HashCodeBuilder(10413, 403929) According to https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/HashCodeBuilder.html#HashCodeBuilder-int-int- Prime numbers are preferred, especially for the multiplier. Both numbers are divisible by 3. Can we use prime numbers? Same for ErasureCodingPoicy#hashCode() return new HashCodeBuilder(925451, 205449) 925451 is divisible by 23. 205449 is divisible by 3.

Andrew Wang added a comment - 13/Apr/17 21:47 Thanks for reviewing, good catch! I thought it just wanted odd numbers. Used this site to generate some 31-bit primes: https://asecuritysite.com/encryption/random3?val=8

Hadoop QA added a comment - 13/Apr/17 23:02 +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 11s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 13s Maven dependency ordering for branch +1 mvninstall 13m 18s trunk passed +1 compile 15m 40s trunk passed +1 checkstyle 1m 55s trunk passed +1 mvnsite 1m 43s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 3m 4s trunk passed +1 javadoc 1m 18s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 1m 8s the patch passed +1 compile 13m 51s the patch passed +1 javac 13m 51s the patch passed -0 checkstyle 1m 53s root: The patch generated 6 new + 8 unchanged - 1 fixed = 14 total (was 9) +1 mvnsite 1m 42s the patch passed +1 mvneclipse 0m 39s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 3m 17s the patch passed +1 javadoc 1m 15s the patch passed +1 unit 8m 56s hadoop-common in the patch passed. +1 unit 1m 30s hadoop-hdfs-client in the patch passed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 74m 17s Subsystem Report/Notes Docker Image:yetus/hadoop:612578f JIRA Issue HDFS-11652 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12863372/HDFS-11652.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 10c79fd84b1e 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 / 0cab572 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/19083/artifact/patchprocess/diff-checkstyle-root.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19083/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19083/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Wei-Chiu Chuang added a comment - 14/Apr/17 19:00 Could you fix the checkstyle warning? +1 after that. Thanks!

Rakesh R added a comment - 16/Apr/17 11:43 Thanks Andrew Wang for the improvement. Just one suggestion, apart from that it looks good to me. In tests, please include Assert.fail for the NPE, IllegalArgumentException conditions like below. try { // source code to verify Assert.fail( "Fails with the given message" ); } catch (NullPointerException e) { }

Andrew Wang added a comment - 17/Apr/17 18:47 Thanks for reviewing Wei-Chiu and Rakesh! New patch attached.

Hadoop QA added a comment - 17/Apr/17 20:03 +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 1m 31s Maven dependency ordering for branch +1 mvninstall 13m 31s trunk passed +1 compile 15m 47s trunk passed +1 checkstyle 1m 55s trunk passed +1 mvnsite 1m 44s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 3m 0s trunk passed +1 javadoc 1m 18s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 1m 10s the patch passed +1 compile 13m 39s the patch passed +1 javac 13m 39s the patch passed +1 checkstyle 1m 53s root: The patch generated 0 new + 8 unchanged - 1 fixed = 8 total (was 9) +1 mvnsite 1m 42s the patch passed +1 mvneclipse 0m 41s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 3m 16s the patch passed +1 javadoc 1m 16s the patch passed +1 unit 8m 17s hadoop-common in the patch passed. +1 unit 1m 23s hadoop-hdfs-client in the patch passed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 75m 3s Subsystem Report/Notes Docker Image:yetus/hadoop:0ac17dc JIRA Issue HDFS-11652 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12863674/HDFS-11652.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 920547266f89 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 / ac3cfdf Default Java 1.8.0_121 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19108/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19108/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Andrew Wang added a comment - 17/Apr/17 23:30 Committed to trunk, thanks again for reviewing Wei-chiu, Rakesh!

Hudson added a comment - 18/Apr/17 00:08 SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11596 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11596/ ) HDFS-11652 . Improve ECSchema and ErasureCodingPolicy toString, hashCode, (wang: rev c0ca785dbb516335ea7e170abb57550251a5d8f6) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/erasurecode/ECSchema.java (add) hadoop-hdfs-project/hadoop-hdfs-client/src/test/java/org/apache/hadoop/hdfs/protocol/TestErasureCodingPolicy.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/erasurecode/TestECSchema.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ErasureCodingPolicy.java