Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-11652

Improve ECSchema and ErasureCodingPolicy toString, hashCode, equals

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha2
    • Fix Version/s: 3.0.0-alpha4
    • Component/s: erasure-coding
    • Labels:
      None

      Description

      Some small cleanups to these methods.

      1. HDFS-11652.001.patch
        10 kB
        Andrew Wang
      2. HDFS-11652.002.patch
        11 kB
        Andrew Wang
      3. HDFS-11652.003.patch
        11 kB
        Andrew Wang
      4. HDFS-11652.004.patch
        11 kB
        Andrew Wang

        Activity

        Hide
        hudson Hudson added a comment -

        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
        Show
        hudson Hudson added a comment - 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
        Hide
        andrew.wang Andrew Wang added a comment -

        Committed to trunk, thanks again for reviewing Wei-chiu, Rakesh!

        Show
        andrew.wang Andrew Wang added a comment - Committed to trunk, thanks again for reviewing Wei-chiu, Rakesh!
        Hide
        hadoopqa Hadoop QA added a comment -
        +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.

        Show
        hadoopqa Hadoop QA added a comment - +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.
        Hide
        andrew.wang Andrew Wang added a comment -

        Thanks for reviewing Wei-Chiu and Rakesh! New patch attached.

        Show
        andrew.wang Andrew Wang added a comment - Thanks for reviewing Wei-Chiu and Rakesh! New patch attached.
        Hide
        rakeshr Rakesh R added a comment -

        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) {
            }
        
        Show
        rakeshr Rakesh R added a comment - 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) { }
        Hide
        jojochuang Wei-Chiu Chuang added a comment -

        Could you fix the checkstyle warning? +1 after that. Thanks!

        Show
        jojochuang Wei-Chiu Chuang added a comment - Could you fix the checkstyle warning? +1 after that. Thanks!
        Hide
        hadoopqa Hadoop QA added a comment -
        +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.

        Show
        hadoopqa Hadoop QA added a comment - +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.
        Hide
        andrew.wang Andrew Wang added a comment -

        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

        Show
        andrew.wang Andrew Wang added a comment - 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
        Hide
        jojochuang Wei-Chiu Chuang added a comment -

        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.

        Show
        jojochuang Wei-Chiu Chuang added a comment - 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.
        Hide
        hadoopqa Hadoop QA added a comment -
        +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.

        Show
        hadoopqa Hadoop QA added a comment - +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.
        Hide
        andrew.wang Andrew Wang added a comment -

        Few mistakes crept in, 002 patch.

        Show
        andrew.wang Andrew Wang added a comment - Few mistakes crept in, 002 patch.
        Hide
        andrew.wang Andrew Wang added a comment -

        Wei-Chiu Chuang you noticed the ID issue over on another JIRA, mind reviewing this one?

        Show
        andrew.wang Andrew Wang added a comment - Wei-Chiu Chuang you noticed the ID issue over on another JIRA, mind reviewing this one?
        Hide
        andrew.wang Andrew Wang added a comment -

        Patch attached.

        • Switched over to using EqualsBuilder and HashBuilder
        • Added unit tests
        • Added "id" field in ECPolicy to toString, equals, hashCode
        Show
        andrew.wang Andrew Wang added a comment - Patch attached. Switched over to using EqualsBuilder and HashBuilder Added unit tests Added "id" field in ECPolicy to toString, equals, hashCode

          People

          • Assignee:
            andrew.wang Andrew Wang
            Reporter:
            andrew.wang Andrew Wang
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development