HBase
  1. HBase
  2. HBASE-4820

Distributed log splitting coding enhancement to make it easier to understand, no semantics change

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.92.0, 0.94.0
    • Fix Version/s: 0.94.0
    • Component/s: wal
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      In reviewing distributed log splitting feature, we found some cosmetic issues. They make the code hard to understand.
      It will be great to fix them. For this issue, there should be no semantic change.

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #17 (See https://builds.apache.org/job/HBase-TRUNK-security/17/)
        HBASE-4820. Distributed log splitting coding enhancement to make it easier to understand, no semantics change. Contributed by Jimmy Xiang.

        todd :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #17 (See https://builds.apache.org/job/HBase-TRUNK-security/17/ ) HBASE-4820 . Distributed log splitting coding enhancement to make it easier to understand, no semantics change. Contributed by Jimmy Xiang. todd : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2503 (See https://builds.apache.org/job/HBase-TRUNK/2503/)
        HBASE-4820. Distributed log splitting coding enhancement to make it easier to understand, no semantics change. Contributed by Jimmy Xiang.

        todd :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2503 (See https://builds.apache.org/job/HBase-TRUNK/2503/ ) HBASE-4820 . Distributed log splitting coding enhancement to make it easier to understand, no semantics change. Contributed by Jimmy Xiang. todd : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java
        Hide
        Todd Lipcon added a comment -

        Committed to trunk (94) only, since 92 is already in an rc phase. We can backport this to 92 easily later if need be.

        Show
        Todd Lipcon added a comment - Committed to trunk (94) only, since 92 is already in an rc phase. We can backport this to 92 easily later if need be.
        Hide
        Todd Lipcon added a comment -

        I'll commit this at the end of the day if there are no objections.

        Show
        Todd Lipcon added a comment - I'll commit this at the end of the day if there are no objections.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3546
        -----------------------------------------------------------

        Ship it!

        • Todd

        On 2011-11-28 19:25:01, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-28 19:25:01)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3546 ----------------------------------------------------------- Ship it! Todd On 2011-11-28 19:25:01, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-28 19:25:01) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        Jimmy Xiang added a comment -

        This patch works for both 0.94 and 0.92. TestSplitLogManager,TestDistributedLogSplitting works for both.
        Please commit the patch to both 0.94 and 0.92.

        Show
        Jimmy Xiang added a comment - This patch works for both 0.94 and 0.92. TestSplitLogManager,TestDistributedLogSplitting works for both. Please commit the patch to both 0.94 and 0.92.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/
        -----------------------------------------------------------

        (Updated 2011-11-28 19:25:01.275541)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Changes
        -------

        Latest diff, upload it again.

        Summary
        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.
        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.
        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054
        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d
        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9
        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1
        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f
        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        Diff: https://reviews.apache.org/r/2895/diff

        Testing
        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-28 19:25:01.275541) Review request for hbase, Todd Lipcon and Jonathan Robie. Changes ------- Latest diff, upload it again. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs (updated) src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        Ted Yu added a comment -

        @Jimmy:
        Please upload latest patch to review board.

        https://builds.apache.org/job/PreCommit-HBASE-Build/367//testReport/ was incomplete. Please run log splitting related tests manually.

        Let's see what Kannan has to say.

        Show
        Ted Yu added a comment - @Jimmy: Please upload latest patch to review board. https://builds.apache.org/job/PreCommit-HBASE-Build/367//testReport/ was incomplete. Please run log splitting related tests manually. Let's see what Kannan has to say.
        Hide
        Jimmy Xiang added a comment -

        Hi Kannan and Ted,

        Do you have any concerns with the new patch?

        Show
        Jimmy Xiang added a comment - Hi Kannan and Ted, Do you have any concerns with the new patch?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12505146/0001-HBASE-4820-Minor-distributed-log-splitting-enhanceme_new.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 8 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated -162 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 67 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:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/367//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/367//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/367//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/12505146/0001-HBASE-4820-Minor-distributed-log-splitting-enhanceme_new.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -162 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 67 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: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/367//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/367//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/367//console This message is automatically generated.
        Hide
        Jimmy Xiang added a comment -

        This one is the latest:

        0001-HBASE-4820-Minor-distributed-log-splitting-enhanceme_new.patch

        Show
        Jimmy Xiang added a comment - This one is the latest: 0001- HBASE-4820 -Minor-distributed-log-splitting-enhanceme_new.patch
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-11-23 22:51:05, Todd Lipcon wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java, line 636

        > <https://reviews.apache.org/r/2895/diff/3/?file=59937#file59937line636>

        >

        > put the edits where?

        • Parse a single hlog and put the edits in entryBuffers
        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3492
        -----------------------------------------------------------

        On 2011-11-23 19:58:09, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-23 19:58:09)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-11-23 22:51:05, Todd Lipcon wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java, line 636 > < https://reviews.apache.org/r/2895/diff/3/?file=59937#file59937line636 > > > put the edits where? Parse a single hlog and put the edits in entryBuffers Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3492 ----------------------------------------------------------- On 2011-11-23 19:58:09, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-23 19:58:09) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3492
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        <https://reviews.apache.org/r/2895/#comment7770>

        put the edits where?

        • Todd

        On 2011-11-23 19:58:09, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-23 19:58:09)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3492 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java < https://reviews.apache.org/r/2895/#comment7770 > put the edits where? Todd On 2011-11-23 19:58:09, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-23 19:58:09) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12504918/0001-HBASE-4820-Minor-distributed-log-splitting-enhanceme.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 8 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated -162 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 66 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.client.TestAdmin
        org.apache.hadoop.hbase.replication.TestReplication

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/351//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/351//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/351//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/12504918/0001-HBASE-4820-Minor-distributed-log-splitting-enhanceme.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -162 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 66 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.client.TestAdmin org.apache.hadoop.hbase.replication.TestReplication Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/351//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/351//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/351//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/
        -----------------------------------------------------------

        (Updated 2011-11-23 19:58:09.915833)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Changes
        -------

        Per Todd's suggestion, the patch is enhanced for easy back porting.

        Summary
        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.
        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.
        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054
        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d
        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9
        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1
        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f
        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        Diff: https://reviews.apache.org/r/2895/diff

        Testing
        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-23 19:58:09.915833) Review request for hbase, Todd Lipcon and Jonathan Robie. Changes ------- Per Todd's suggestion, the patch is enhanced for easy back porting. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs (updated) src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 2101054 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java d7a648d src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        Todd Lipcon added a comment -

        Can we meet in the middle with this patch? A few suggestions that would make the patch more trivial to review:

        • don't do the whitespace-only fixes in parts of the code you're not touching
        • don't expand out the "import foo.*"s
        • don't move the callback code to different parts of the file
        • do fix variable names to conform to style, remove dead code, add javadoc, rename classes, etc.

        This should make the patch very easy to look over and make sure it doesn't break anything. It'll then be easy for FB to pull it into their branch if they want.

        Show
        Todd Lipcon added a comment - Can we meet in the middle with this patch? A few suggestions that would make the patch more trivial to review: don't do the whitespace-only fixes in parts of the code you're not touching don't expand out the "import foo.*"s don't move the callback code to different parts of the file do fix variable names to conform to style, remove dead code, add javadoc, rename classes, etc. This should make the patch very easy to look over and make sure it doesn't break anything. It'll then be easy for FB to pull it into their branch if they want.
        Hide
        Jimmy Xiang added a comment -

        I think Jon has a great point. In porting this feature to CDH, I spent quite some time to understand it.

        To Kannan's concern, the coding change for this Jira should be easy to back port since you already have the original change.

        To someone else who doesn't have the original change, this Jira is supposed to make it easier to understand and port.

        Show
        Jimmy Xiang added a comment - I think Jon has a great point. In porting this feature to CDH, I spent quite some time to understand it. To Kannan's concern, the coding change for this Jira should be easy to back port since you already have the original change. To someone else who doesn't have the original change, this Jira is supposed to make it easier to understand and port.
        Hide
        Jonathan Hsieh added a comment -

        @Kannan, I'm looking at this from the point of view of someone who recently spent a many hours reviewing the dist log splitting patches in aggregate and may be responsible for fixing issues if it has problems. I had a harder time than I'd prefer, and will likely have the same problem again if there are problems in the future. Doing a little bit of semantics preserving changes such as making var/method/class names more descriptive and encapsulating pieces would go a long way to make the code more easily and quickly understandable by more people.

        Are you suggesting splitting these changes into smaller pieces such as:

        • add better exception error messages.
        • consolidate calls only used once. Ex: async callbacks submethods; inline finishInitailize into SLM's constructor
        • rename vague methods. ex: installTask(String taskName) might be better as enqueueSplitLog(String logPath); handleDeadWorker might be better as blacklistDeadWorker; 'exec(String name, Progressable)' might be better as 'split(String logfilename, Progressable)'
        • rename vague classes. ex: Task to SplitTask, TaskBatch to SplitTaskState/SplitTaskContext
        • correct comments to be consistent with code (comments in SplitLogWorker talks about SUCCESS state which acutally is DONE state).
        Show
        Jonathan Hsieh added a comment - @Kannan, I'm looking at this from the point of view of someone who recently spent a many hours reviewing the dist log splitting patches in aggregate and may be responsible for fixing issues if it has problems. I had a harder time than I'd prefer, and will likely have the same problem again if there are problems in the future. Doing a little bit of semantics preserving changes such as making var/method/class names more descriptive and encapsulating pieces would go a long way to make the code more easily and quickly understandable by more people. Are you suggesting splitting these changes into smaller pieces such as: add better exception error messages. consolidate calls only used once. Ex: async callbacks submethods; inline finishInitailize into SLM's constructor rename vague methods. ex: installTask(String taskName) might be better as enqueueSplitLog(String logPath); handleDeadWorker might be better as blacklistDeadWorker; 'exec(String name, Progressable)' might be better as 'split(String logfilename, Progressable)' rename vague classes. ex: Task to SplitTask, TaskBatch to SplitTaskState/SplitTaskContext correct comments to be consistent with code (comments in SplitLogWorker talks about SUCCESS state which acutally is DONE state).
        Hide
        Ted Yu added a comment -

        I agree with Kannan that we should reduce the number of places where refactor is done to make porting easier.

        Currently SplitLogManager.splitLogDistributed(final List<Path> logDirs) creates MonitoredTask to display status.
        This results in repetitive display of the following form on 60010/master-status:

        Doing distributed log split in [hdfs://...-splitting]
        

        We should make log splitting status display cleaner.

        Show
        Ted Yu added a comment - I agree with Kannan that we should reduce the number of places where refactor is done to make porting easier. Currently SplitLogManager.splitLogDistributed(final List<Path> logDirs) creates MonitoredTask to display status. This results in repetitive display of the following form on 60010/master-status: Doing distributed log split in [hdfs: //...-splitting] We should make log splitting status display cleaner.
        Hide
        Kannan Muthukkaruppan added a comment -

        This type of code factoring changes will make it harder for 89-fb/our internal branch changes to stay in sync with trunk; and to push/pull patches between the two revs. But agree that that can't be the reason to block all code factor/improvements. So those have to be evaluated on a case to case basis. Do we think these changes and code moves are worth it? [I have only looked at the specific changes superficially, but wanted to express the concern at least so that someone who has reviewed in detail can comment if this change is a must.]

        Show
        Kannan Muthukkaruppan added a comment - This type of code factoring changes will make it harder for 89-fb/our internal branch changes to stay in sync with trunk; and to push/pull patches between the two revs. But agree that that can't be the reason to block all code factor/improvements. So those have to be evaluated on a case to case basis. Do we think these changes and code moves are worth it? [I have only looked at the specific changes superficially, but wanted to express the concern at least so that someone who has reviewed in detail can comment if this change is a must.]
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java, line 271

        > <https://reviews.apache.org/r/2895/diff/1/?file=59537#file59537line271>

        >

        > handleDeadWorkers would be a better method name.

        Yes, that's right.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 431

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line431>

        >

        > retry_count is the remaining count. This log message should be clearer.

        That's right

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 480

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line480>

        >

        > We should say 'remaining retries='

        Fixed.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 453

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line453>

        >

        > Can we implement this item now ?

        We can do it in another jira.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 671

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line671>

        >

        > Please adjust indentation.

        That's right.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 210

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line210>

        >

        > Please remove white space.

        I assume you suggest we should not use tab. Please correct me if I am wrong.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 952

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line952>

        >

        > Please adjust indentation for these 4 lines.

        Fixed. Replaced tabs with spaces.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 965

        > <https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line965>

        >

        > Should read 'splitlog workers'

        fixed.

        On 2011-11-21 03:29:17, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java, line 648

        > <https://reviews.apache.org/r/2895/diff/1/?file=59540#file59540line648>

        >

        > Adjust indentation, please.

        fixed.

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3385
        -----------------------------------------------------------

        On 2011-11-21 02:06:29, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-21 02:06:29)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java, line 271 > < https://reviews.apache.org/r/2895/diff/1/?file=59537#file59537line271 > > > handleDeadWorkers would be a better method name. Yes, that's right. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 431 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line431 > > > retry_count is the remaining count. This log message should be clearer. That's right On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 480 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line480 > > > We should say 'remaining retries=' Fixed. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 453 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line453 > > > Can we implement this item now ? We can do it in another jira. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 671 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line671 > > > Please adjust indentation. That's right. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 210 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line210 > > > Please remove white space. I assume you suggest we should not use tab. Please correct me if I am wrong. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 952 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line952 > > > Please adjust indentation for these 4 lines. Fixed. Replaced tabs with spaces. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java, line 965 > < https://reviews.apache.org/r/2895/diff/1/?file=59538#file59538line965 > > > Should read 'splitlog workers' fixed. On 2011-11-21 03:29:17, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java, line 648 > < https://reviews.apache.org/r/2895/diff/1/?file=59540#file59540line648 > > > Adjust indentation, please. fixed. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3385 ----------------------------------------------------------- On 2011-11-21 02:06:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-21 02:06:29) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8 Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-11-21 04:54:00, ramkrishna vasudevan wrote:

        > src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 1530

        > <https://reviews.apache.org/r/2895/diff/1/?file=59536#file59536line1530>

        >

        > Can we make this msg more clear.

        > Something like

        > Unexpected state : statename.. Cannot transit znode state from : currentState to OFFLINE.

        You got it.

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3388
        -----------------------------------------------------------

        On 2011-11-21 02:06:29, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-21 02:06:29)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-11-21 04:54:00, ramkrishna vasudevan wrote: > src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 1530 > < https://reviews.apache.org/r/2895/diff/1/?file=59536#file59536line1530 > > > Can we make this msg more clear. > Something like > Unexpected state : statename.. Cannot transit znode state from : currentState to OFFLINE. You got it. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3388 ----------------------------------------------------------- On 2011-11-21 02:06:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-21 02:06:29) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8 Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/
        -----------------------------------------------------------

        (Updated 2011-11-21 18:22:03.402105)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Changes
        -------

        Updated patch diff after changes per review.

        Summary
        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.
        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.
        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653
        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c
        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9
        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1
        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f
        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8

        Diff: https://reviews.apache.org/r/2895/diff

        Testing
        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-21 18:22:03.402105) Review request for hbase, Todd Lipcon and Jonathan Robie. Changes ------- Updated patch diff after changes per review. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs (updated) src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8 Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3388
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/2895/#comment7574>

        Can we make this msg more clear.
        Something like
        Unexpected state : statename.. Cannot transit znode state from : currentState to OFFLINE.

        • ramkrishna

        On 2011-11-21 02:06:29, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-21 02:06:29)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3388 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/2895/#comment7574 > Can we make this msg more clear. Something like Unexpected state : statename.. Cannot transit znode state from : currentState to OFFLINE. ramkrishna On 2011-11-21 02:06:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-21 02:06:29) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8 Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/#review3385
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
        <https://reviews.apache.org/r/2895/#comment7563>

        handleDeadWorkers would be a better method name.

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7562>

        Please remove white space.

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7564>

        retry_count is the remaining count. This log message should be clearer.

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7565>

        Can we implement this item now ?

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7566>

        We should say 'remaining retries='

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7567>

        Please adjust indentation.

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7571>

        Please adjust indentation for these 4 lines.

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
        <https://reviews.apache.org/r/2895/#comment7570>

        Should read 'splitlog workers'

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        <https://reviews.apache.org/r/2895/#comment7572>

        Adjust indentation, please.

        • Ted

        On 2011-11-21 02:06:29, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2895/

        -----------------------------------------------------------

        (Updated 2011-11-21 02:06:29)

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary

        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.

        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.

        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c

        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1

        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f

        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec

        src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8

        Diff: https://reviews.apache.org/r/2895/diff

        Testing

        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/#review3385 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java < https://reviews.apache.org/r/2895/#comment7563 > handleDeadWorkers would be a better method name. src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7562 > Please remove white space. src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7564 > retry_count is the remaining count. This log message should be clearer. src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7565 > Can we implement this item now ? src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7566 > We should say 'remaining retries=' src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7567 > Please adjust indentation. src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7571 > Please adjust indentation for these 4 lines. src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java < https://reviews.apache.org/r/2895/#comment7570 > Should read 'splitlog workers' src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java < https://reviews.apache.org/r/2895/#comment7572 > Adjust indentation, please. Ted On 2011-11-21 02:06:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- (Updated 2011-11-21 02:06:29) Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs ----- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8 Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2895/
        -----------------------------------------------------------

        Review request for hbase, Todd Lipcon and Jonathan Robie.

        Summary
        -------

        Distributed log splitting coding enhancement to make it easier to understand, no semantics change.
        It is some issue raised during the code review in back porting this feature to CDH.

        This addresses bug HBASE-4820.
        https://issues.apache.org/jira/browse/HBASE-4820

        Diffs


        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653
        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c
        src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9
        src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1
        src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f
        src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec
        src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8

        Diff: https://reviews.apache.org/r/2895/diff

        Testing
        -------

        Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change.

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2895/ ----------------------------------------------------------- Review request for hbase, Todd Lipcon and Jonathan Robie. Summary ------- Distributed log splitting coding enhancement to make it easier to understand, no semantics change. It is some issue raised during the code review in back porting this feature to CDH. This addresses bug HBASE-4820 . https://issues.apache.org/jira/browse/HBASE-4820 Diffs src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f7ef653 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b9a3a2c src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 7dd67e9 src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 1d329b0 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 21747b1 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 51daa1f src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java c8684ec src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 84d76e8 Diff: https://reviews.apache.org/r/2895/diff Testing ------- Ran unit tests. Non-flaky tests are green. Two client tests didn't pass, which are not related to this change. Thanks, Jimmy
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12504321/0001-HBASE-4820-Distributed-log-splitting-coding-enhancement-to-make----it-easier-to-understand%2C-no-semantics-change..patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 12 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/303//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/12504321/0001-HBASE-4820-Distributed-log-splitting-coding-enhancement-to-make----it-easier-to-understand%2C-no-semantics-change..patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/303//console This message is automatically generated.

          People

          • Assignee:
            Jimmy Xiang
            Reporter:
            Jimmy Xiang
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development