Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.94.0
    • Fix Version/s: 0.95.0
    • Component/s: documentation
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      We should rename "hbase.skip.errors", used in HRegion.java for skipping errors when replaying edits. It should probably be something more like "hbase.hregion.edits.replay.skip.errors" or so.

      1. HBASE-5151.patch
        2 kB
        Harsh J
      2. HBASE-5151.patch
        2 kB
        Harsh J
      3. HBASE-5151.amend.patch
        0.8 kB
        Harsh J
      4. HBASE-5151.patch
        2 kB
        Harsh J
      5. HBASE-5151.amend.wrapped.patch
        0.9 kB
        Harsh J

        Activity

        Hide
        stack added a comment -

        +1

        Show
        stack added a comment - +1
        Hide
        Harsh J added a comment -

        This patch deprecates "hbase.skip.errors" in favor of the new style config mentioned above.

        Ideally I'd have used Configuration.addDeprecatedKeys but thats sadly available only in Hadoop 2.x onwards. I think it will do HBase a good deal someday in future to shed off multiple Hadoop deps to a single, rolling one alone. That day, we can have proper deprecation features in HBaseConfiguration without duplicating code or hacking around, or even having extra code like this patch required.

        Show
        Harsh J added a comment - This patch deprecates "hbase.skip.errors" in favor of the new style config mentioned above. Ideally I'd have used Configuration.addDeprecatedKeys but thats sadly available only in Hadoop 2.x onwards. I think it will do HBase a good deal someday in future to shed off multiple Hadoop deps to a single, rolling one alone. That day, we can have proper deprecation features in HBaseConfiguration without duplicating code or hacking around, or even having extra code like this patch required.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12535626/HBASE-5151.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +0 tests included. The patch appears to be a documentation patch that doesn't require tests.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings).

        -1 findbugs. The patch appears to introduce 7 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.master.TestAssignmentManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535626/HBASE-5151.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +0 tests included. The patch appears to be a documentation patch that doesn't require tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings). -1 findbugs. The patch appears to introduce 7 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestAssignmentManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2352//console This message is automatically generated.
        Hide
        stack added a comment -

        @Harsh Patch looks good. Does the above test fail for you?

        Show
        stack added a comment - @Harsh Patch looks good. Does the above test fail for you?
        Hide
        Harsh J added a comment -

        It passes for me:

         T E S T S
        -------------------------------------------------------
        Running org.apache.hadoop.hbase.master.TestAssignmentManager
        Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 34.817 sec
        
        Results :
        
        Tests run: 12, Failures: 0, Errors: 0, Skipped: 0
        

        However I forgot to fix the additional logger string reference to the old prop so I did that now in this new patch. Sorry!

        Show
        Harsh J added a comment - It passes for me: T E S T S ------------------------------------------------------- Running org.apache.hadoop.hbase.master.TestAssignmentManager Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 34.817 sec Results : Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 However I forgot to fix the additional logger string reference to the old prop so I did that now in this new patch. Sorry!
        Hide
        stack added a comment -

        Committed to trunk. Thanks for the patch Harsh.

        Show
        stack added a comment - Committed to trunk. Thanks for the patch Harsh.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #3118 (See https://builds.apache.org/job/HBase-TRUNK/3118/)
        HBASE-5151 Rename hbase.skip.errors in HRegion as it is too general-sounding (Revision 1360172)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #3118 (See https://builds.apache.org/job/HBase-TRUNK/3118/ ) HBASE-5151 Rename hbase.skip.errors in HRegion as it is too general-sounding (Revision 1360172) Result = FAILURE stack : Files : /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        Ted Yu added a comment -

        In trunk build 3118:

        [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.0.2:compile (default-compile) on project hbase-server: Compilation failure: Compilation failure:
        [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2854,63] ')' expected
        [ERROR] 
        [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,19] not a statement
        [ERROR] 
        [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,22] ';' expected
        [ERROR] 
        [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,24] not a statement
        [ERROR] 
        [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,25] ';' expected
        
        Show
        Ted Yu added a comment - In trunk build 3118: [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.0.2:compile ( default -compile) on project hbase-server: Compilation failure: Compilation failure: [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2854,63] ')' expected [ERROR] [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,19] not a statement [ERROR] [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,22] ';' expected [ERROR] [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,24] not a statement [ERROR] [ERROR] /home/jenkins/jenkins-slave/workspace/HBase-TRUNK/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:[2855,25] ';' expected
        Hide
        Harsh J added a comment -

        GAH, sorry guys! Amendment attached (.amend.patch, carries just fix of "+", delta).

        Also attached a completely whole proper diff - whichever you may need to apply (revert and apply vs. amend apply).

        Show
        Harsh J added a comment - GAH, sorry guys! Amendment attached (.amend.patch, carries just fix of "+", delta). Also attached a completely whole proper diff - whichever you may need to apply (revert and apply vs. amend apply).
        Hide
        Ted Yu added a comment -

        @Harsh:
        In the future, please version patches with a number.
        Now we have three attachments with the same name.

        Show
        Ted Yu added a comment - @Harsh: In the future, please version patches with a number. Now we have three attachments with the same name.
        Hide
        Harsh J added a comment -

        Sure, will do. I thought it was practice to rely on JIRA's coloring of the older versions of the same name as gray and the newer as blue. Thats what we've followed on HADOOP mostly, so its become a habit, sorry.

        Show
        Harsh J added a comment - Sure, will do. I thought it was practice to rely on JIRA's coloring of the older versions of the same name as gray and the newer as blue. Thats what we've followed on HADOOP mostly, so its become a habit, sorry.
        Hide
        Ted Yu added a comment -

        The line being corrected in addendum is too long.

        Please attach another addendum which wraps that line.

        Thanks

        Show
        Ted Yu added a comment - The line being corrected in addendum is too long. Please attach another addendum which wraps that line. Thanks
        Hide
        Harsh J added a comment -

        Here it is.

        Will be more careful in future, apologies.

        Show
        Harsh J added a comment - Here it is. Will be more careful in future, apologies.
        Hide
        stack added a comment -

        @Harsh No need to apologize. Thanks for fast turn around. Applying the amendment.

        Show
        stack added a comment - @Harsh No need to apologize. Thanks for fast turn around. Applying the amendment.
        Hide
        stack added a comment -

        I applied the amendment. Thanks Harsh.

        Show
        stack added a comment - I applied the amendment. Thanks Harsh.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #3119 (See https://builds.apache.org/job/HBase-TRUNK/3119/)
        HBASE-5151 Rename hbase.skip.errors in HRegion as it is too general-sounding (Revision 1360384)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #3119 (See https://builds.apache.org/job/HBase-TRUNK/3119/ ) HBASE-5151 Rename hbase.skip.errors in HRegion as it is too general-sounding (Revision 1360384) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        Ted Yu added a comment -

        It turns out that the first patch was syntactically correct.
        Harsh added something in patch v2 which wouldn't pass compilation.

        Currently Hadoop QA wouldn't post back if there is compilation error.
        However, Stack wasn't aware of the above and integrated patch v2.

        This is another reason we need versioning in patch filenames so that such mistakes can be more easily avoided.

        Show
        Ted Yu added a comment - It turns out that the first patch was syntactically correct. Harsh added something in patch v2 which wouldn't pass compilation. Currently Hadoop QA wouldn't post back if there is compilation error. However, Stack wasn't aware of the above and integrated patch v2. This is another reason we need versioning in patch filenames so that such mistakes can be more easily avoided.
        Hide
        Harsh J added a comment -

        This is another reason we need versioning in patch filenames so that such mistakes can be more easily avoided.

        Understood Ted. When posting patches over HBASE in future, I'll make sure to number them every revision. Thanks!

        Show
        Harsh J added a comment - This is another reason we need versioning in patch filenames so that such mistakes can be more easily avoided. Understood Ted. When posting patches over HBASE in future, I'll make sure to number them every revision. Thanks!
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            Harsh J
            Reporter:
            Harsh J
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development