HBase
  1. HBase
  2. HBASE-5328

Small changes to Master to make it more testable

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Here are some small changes in Master that make it more testable. Included tests stand up a Master and then fake it into thinking that three regionservers are registering making master assign root and meta, etc.

      1. 5328.txt
        62 kB
        stack
      2. 5328v2.txt
        59 kB
        stack
      3. 5328v2.txt
        59 kB
        stack
      4. 5328v3.txt
        60 kB
        stack
      5. 5328v4.txt
        60 kB
        stack
      6. 5328v8.txt
        94 kB
        stack
      7. 5328v12.txt
        60 kB
        stack
      8. 5328-addendum.txt
        1 kB
        stack

        Activity

        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2692 (See https://builds.apache.org/job/HBase-TRUNK/2692/)
        HBASE-5328 Small changes to Master to make it more testable (Revision 1303648)
        HBASE-5328 Small changes to Master to make it more testable (Revision 1303626)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java

        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/Mocking.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2692 (See https://builds.apache.org/job/HBase-TRUNK/2692/ ) HBASE-5328 Small changes to Master to make it more testable (Revision 1303648) HBASE-5328 Small changes to Master to make it more testable (Revision 1303626) Result = SUCCESS stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/Mocking.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        Sorry, meant to have this said this after the first review.

        • jmhsieh

        On 2012-03-21 23:18:23, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 23:18:23)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6223 ----------------------------------------------------------- Ship it! Sorry, meant to have this said this after the first review. jmhsieh On 2012-03-21 23:18:23, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 23:18:23) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-22 00:25:30, jmhsieh wrote:

        > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 493

        > <https://reviews.apache.org/r/4436/diff/1/?file=94295#file94295line493>

        >

        > nit: comment is wrong for change.

        I fixed this in an addendum.

        On 2012-03-22 00:25:30, jmhsieh wrote:

        > src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java, line 165

        > <https://reviews.apache.org/r/4436/diff/1/?file=94300#file94300line165>

        >

        > was: remove all instances of "TODO Auto-generated method stub"?

        >

        > Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?)

        Michael Stack wrote:

        Its a mock. I want default, basic behaviors.

        jmhsieh wrote:

        I'm not going to push too hard here, but leaving the auto gen stub generally tells me "incomplete code" instead of telling "basic behavior" or "not default behavior".

        I looked at this this morning again since it reviewers balk. I tried to come up w/ text to put in place of what was auto-generated but anything I conjured seemed inauthentic compared to what the machine generated so I am going to just leave it (It shouldn't be overridden, at least not currently, not until we need the mock to do more, and neither do I want to throw runtime exceptions, etc).

        On 2012-03-22 00:25:30, jmhsieh wrote:

        > src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java, line 114

        > <https://reviews.apache.org/r/4436/diff/1/?file=94303#file94303line114>

        >

        > make these joins have a timeout (prevent hanging tests?)

        Done in an addendum.

        • Michael

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

        On 2012-03-21 23:18:23, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 23:18:23)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-22 00:25:30, jmhsieh wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 493 > < https://reviews.apache.org/r/4436/diff/1/?file=94295#file94295line493 > > > nit: comment is wrong for change. I fixed this in an addendum. On 2012-03-22 00:25:30, jmhsieh wrote: > src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java, line 165 > < https://reviews.apache.org/r/4436/diff/1/?file=94300#file94300line165 > > > was: remove all instances of "TODO Auto-generated method stub"? > > Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?) Michael Stack wrote: Its a mock. I want default, basic behaviors. jmhsieh wrote: I'm not going to push too hard here, but leaving the auto gen stub generally tells me "incomplete code" instead of telling "basic behavior" or "not default behavior". I looked at this this morning again since it reviewers balk. I tried to come up w/ text to put in place of what was auto-generated but anything I conjured seemed inauthentic compared to what the machine generated so I am going to just leave it (It shouldn't be overridden, at least not currently, not until we need the mock to do more, and neither do I want to throw runtime exceptions, etc). On 2012-03-22 00:25:30, jmhsieh wrote: > src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java, line 114 > < https://reviews.apache.org/r/4436/diff/1/?file=94303#file94303line114 > > > make these joins have a timeout (prevent hanging tests?) Done in an addendum. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6188 ----------------------------------------------------------- On 2012-03-21 23:18:23, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 23:18:23) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #145 (See https://builds.apache.org/job/HBase-TRUNK-security/145/)
        HBASE-5328 Small changes to Master to make it more testable (Revision 1303648)
        HBASE-5328 Small changes to Master to make it more testable (Revision 1303626)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java

        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/Mocking.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #145 (See https://builds.apache.org/job/HBase-TRUNK-security/145/ ) HBASE-5328 Small changes to Master to make it more testable (Revision 1303648) HBASE-5328 Small changes to Master to make it more testable (Revision 1303626) Result = SUCCESS stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/Mocking.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-22 00:25:30, jmhsieh wrote:

        > src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java, line 165

        > <https://reviews.apache.org/r/4436/diff/1/?file=94300#file94300line165>

        >

        > was: remove all instances of "TODO Auto-generated method stub"?

        >

        > Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?)

        Michael Stack wrote:

        Its a mock. I want default, basic behaviors.

        I'm not going to push too hard here, but leaving the auto gen stub generally tells me "incomplete code" instead of telling "basic behavior" or "not default behavior".

        • jmhsieh

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

        On 2012-03-21 23:18:23, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 23:18:23)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-22 00:25:30, jmhsieh wrote: > src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java, line 165 > < https://reviews.apache.org/r/4436/diff/1/?file=94300#file94300line165 > > > was: remove all instances of "TODO Auto-generated method stub"? > > Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?) Michael Stack wrote: Its a mock. I want default, basic behaviors. I'm not going to push too hard here, but leaving the auto gen stub generally tells me "incomplete code" instead of telling "basic behavior" or "not default behavior". jmhsieh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6188 ----------------------------------------------------------- On 2012-03-21 23:18:23, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 23:18:23) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        stack added a comment -

        Committed 5238-addendum.txt against trunk.

        Show
        stack added a comment - Committed 5238-addendum.txt against trunk.
        Hide
        stack added a comment -

        Addendum to address two issues raised by Jon up on RB.

        Show
        stack added a comment - Addendum to address two issues raised by Jon up on RB.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-22 00:25:30, jmhsieh wrote:

        >

        Let me add addendum to address two of your feedbacks below Jon.

        On 2012-03-22 00:25:30, jmhsieh wrote:

        > src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java, line 165

        > <https://reviews.apache.org/r/4436/diff/1/?file=94300#file94300line165>

        >

        > was: remove all instances of "TODO Auto-generated method stub"?

        >

        > Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?)

        Its a mock. I want default, basic behaviors.

        • Michael

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

        On 2012-03-21 23:18:23, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 23:18:23)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-22 00:25:30, jmhsieh wrote: > Let me add addendum to address two of your feedbacks below Jon. On 2012-03-22 00:25:30, jmhsieh wrote: > src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java, line 165 > < https://reviews.apache.org/r/4436/diff/1/?file=94300#file94300line165 > > > was: remove all instances of "TODO Auto-generated method stub"? > > Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?) Its a mock. I want default, basic behaviors. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6188 ----------------------------------------------------------- On 2012-03-21 23:18:23, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 23:18:23) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4436/#comment13331>

        nit: comment is wrong for change.

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13333>

        was: remove all instances of "TODO Auto-generated method stub"?

        Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?)

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        <https://reviews.apache.org/r/4436/#comment13360>

        make these joins have a timeout (prevent hanging tests?)

        • jmhsieh

        On 2012-03-21 23:18:23, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 23:18:23)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6188 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4436/#comment13331 > nit: comment is wrong for change. src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13333 > was: remove all instances of "TODO Auto-generated method stub"? Replace with a comment to say this should be overridden (or throw some sort of RuntimeException?) src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java < https://reviews.apache.org/r/4436/#comment13360 > make these joins have a timeout (prevent hanging tests?) jmhsieh On 2012-03-21 23:18:23, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 23:18:23) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        stack added a comment -

        Thanks for the reviews lads. Committed to trunk.

        Show
        stack added a comment - Thanks for the reviews lads. Committed to trunk.
        Hide
        Jean-Daniel Cryans added a comment -

        +1 on v12

        Show
        Jean-Daniel Cryans added a comment - +1 on v12
        Hide
        stack added a comment -

        Cleanup

        Show
        stack added a comment - Cleanup
        Hide
        stack added a comment -

        What I'll apply. Includes rename of methods in Mocking that Ted and Lars suggested... renamed setGet as setGetResult, etc.

        Show
        stack added a comment - What I'll apply. Includes rename of methods in Mocking that Ted and Lars suggested... renamed setGet as setGetResult, etc.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-03-21 23:18:23.112113)

        Review request for hbase.

        Changes
        -------

        Address Ted comments.

        Summary
        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        Make this class public so its waitForRoot(long) can be used by HMaster.
        Remove the stalling waitForRoot no arg.
        M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        Javadoc. Add check if stopped flag cycling waiting on assignment.
        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        Moved check if schema change flag out to a method rather than
        have it on tail of constructor.
        Moved other initialization stuff like get of assignment manager
        and server manager out into methods so could be intercepted by
        tests and mocking.
        Change how we wait on root so we sleep 100ms at a time and always
        check stopped flag rather than block for ever.
        Added more checking if stopped flag.
        Added flag for when rpc server is up, mostly for tests.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
        Unused import.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        Comment.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
        Remove unused code.
        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        Change how we wait on root. DOn't use removed method.
        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.
        https://issues.apache.org/jira/browse/hbase-5328

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f
        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31
        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01
        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604
        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a
        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION
        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a
        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing
        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 23:18:23.112113) Review request for hbase. Changes ------- Address Ted comments. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs (updated) src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        See some minor comment inline.

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        <https://reviews.apache.org/r/4436/#comment13353>

        Nice.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4436/#comment13354>

        This would block forever before?
        Ah never mind, I see below, it's for a stopped master.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4436/#comment13355>

        Nice. Let's the master stop if it can't assign root.

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        <https://reviews.apache.org/r/4436/#comment13356>

        cool

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13357>

        bunch of whitespace in here

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13358>

        Maybe call this setGetResult?

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13359>

        Ditto, setNextResults

        • Lars

        On 2012-03-21 19:50:51, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 19:50:51)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6197 ----------------------------------------------------------- Ship it! See some minor comment inline. src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java < https://reviews.apache.org/r/4436/#comment13353 > Nice. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4436/#comment13354 > This would block forever before? Ah never mind, I see below, it's for a stopped master. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4436/#comment13355 > Nice. Let's the master stop if it can't assign root. src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java < https://reviews.apache.org/r/4436/#comment13356 > cool src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13357 > bunch of whitespace in here src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13358 > Maybe call this setGetResult? src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13359 > Ditto, setNextResults Lars On 2012-03-21 19:50:51, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 19:50:51) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Looks good overall.

        Minor polishing would be better.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4436/#comment13343>

        Can this debug log be unified with the one in the if block ?

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        <https://reviews.apache.org/r/4436/#comment13344>

        Should javadoc be added for this change ?

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13345>

        Should another 'Map ' be added before 'rows to ' ?

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13346>

        It is first time I see a method name like this

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13348>

        Can these be removed ?

        • Ted

        On 2012-03-21 19:50:51, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 19:50:51)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6194 ----------------------------------------------------------- Looks good overall. Minor polishing would be better. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4436/#comment13343 > Can this debug log be unified with the one in the if block ? src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java < https://reviews.apache.org/r/4436/#comment13344 > Should javadoc be added for this change ? src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13345 > Should another 'Map ' be added before 'rows to ' ? src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13346 > It is first time I see a method name like this src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13348 > Can these be removed ? Ted On 2012-03-21 19:50:51, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 19:50:51) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Addressed the comments. Going to commit.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4436/#comment13350>

        Changed it though this not my change. I just moved this log into a method.

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        <https://reviews.apache.org/r/4436/#comment13351>

        No. Its a protected method in a test!

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        <https://reviews.apache.org/r/4436/#comment13352>

        No. Makes it obvious they are not default behavior.

        • Michael

        On 2012-03-21 19:50:51, Michael Stack wrote:

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

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

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

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

        (Updated 2012-03-21 19:50:51)

        Review request for hbase.

        Summary

        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java

        Make this class public so its waitForRoot(long) can be used by HMaster.

        Remove the stalling waitForRoot no arg.

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

        Javadoc. Add check if stopped flag cycling waiting on assignment.

        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java

        Moved check if schema change flag out to a method rather than

        have it on tail of constructor.

        Moved other initialization stuff like get of assignment manager

        and server manager out into methods so could be intercepted by

        tests and mocking.

        Change how we wait on root so we sleep 100ms at a time and always

        check stopped flag rather than block for ever.

        Added more checking if stopped flag.

        Added flag for when rpc server is up, mostly for tests.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java

        Unused import.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java

        Comment.

        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java

        Remove unused code.

        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java

        Change how we wait on root. DOn't use removed method.

        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.

        https://issues.apache.org/jira/browse/hbase-5328

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604

        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a

        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01

        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31

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

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb

        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97

        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing

        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/#review6195 ----------------------------------------------------------- Addressed the comments. Going to commit. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4436/#comment13350 > Changed it though this not my change. I just moved this log into a method. src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java < https://reviews.apache.org/r/4436/#comment13351 > No. Its a protected method in a test! src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java < https://reviews.apache.org/r/4436/#comment13352 > No. Makes it obvious they are not default behavior. Michael On 2012-03-21 19:50:51, Michael Stack wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- (Updated 2012-03-21 19:50:51) Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs ----- src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        stack added a comment -

        Ok if I commit this?

        Show
        stack added a comment - Ok if I commit this?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12519314/5328v3.txt
        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 javadoc. The javadoc tool did not generate any warning messages.

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

        -1 findbugs. The patch appears to introduce 2 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 passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1245//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1245//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1245//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/12519314/5328v3.txt 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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 2 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1245//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1245//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1245//console This message is automatically generated.
        Hide
        stack added a comment -

        I think it ran old version of patch (thats what failed and it passes locally)

        Show
        stack added a comment - I think it ran old version of patch (thats what failed and it passes locally)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12519294/5328v2.txt
        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 javadoc. The javadoc tool did not generate any warning messages.

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

        -1 findbugs. The patch appears to introduce 2 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.catalog.TestCatalogTracker

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1240//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1240//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1240//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/12519294/5328v2.txt 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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 2 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.catalog.TestCatalogTracker Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1240//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1240//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1240//console This message is automatically generated.
        Hide
        stack added a comment -

        Address inane extra white space not added by my patch.

        Fix broke test Ted found.

        Show
        stack added a comment - Address inane extra white space not added by my patch. Fix broke test Ted found.
        Hide
        stack added a comment -

        The above lint complaint is not caused by my patch. Will fix anyways.

        Show
        stack added a comment - The above lint complaint is not caused by my patch. Will fix anyways.
        Hide
        Ted Yu added a comment -

        Snippet from 'arc lint' output:

        >>> Lint for src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java:
        
           Error  (TXT6) Trailing Whitespace
            This line contains trailing whitespace.
        
                     144 
                     145   /**
                     146    * Test a balance going on at same time as a master failover
            >>> -    147    * 
                +           *
                     148    * @throws IOException
                     149    * @throws KeeperException
                     150    * @throws InterruptedException
        
           Error  (TXT6) Trailing Whitespace
            This line contains trailing whitespace.
        
                     481    * @param region region to be created as offline
                     482    * @param serverName server event originates from
                     483    * @return Version of znode created.
            >>> -    484    * @throws KeeperException 
                +           * @throws KeeperException
                     485    * @throws IOException 
                     486    */
                     487   // Copied from SplitTransaction rather than open the method over there in
        
           Error  (TXT6) Trailing Whitespace
            This line contains trailing whitespace.
        
                     482    * @param serverName server event originates from
                     483    * @return Version of znode created.
                     484    * @throws KeeperException 
            >>> -    485    * @throws IOException 
                +           * @throws IOException
                     486    */
                     487   // Copied from SplitTransaction rather than open the method over there in
                     488   // the regionserver package.
        
        Show
        Ted Yu added a comment - Snippet from 'arc lint' output: >>> Lint for src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java: Error (TXT6) Trailing Whitespace This line contains trailing whitespace. 144 145 /** 146 * Test a balance going on at same time as a master failover >>> - 147 * + * 148 * @ throws IOException 149 * @ throws KeeperException 150 * @ throws InterruptedException Error (TXT6) Trailing Whitespace This line contains trailing whitespace. 481 * @param region region to be created as offline 482 * @param serverName server event originates from 483 * @ return Version of znode created. >>> - 484 * @ throws KeeperException + * @ throws KeeperException 485 * @ throws IOException 486 */ 487 // Copied from SplitTransaction rather than open the method over there in Error (TXT6) Trailing Whitespace This line contains trailing whitespace. 482 * @param serverName server event originates from 483 * @ return Version of znode created. 484 * @ throws KeeperException >>> - 485 * @ throws IOException + * @ throws IOException 486 */ 487 // Copied from SplitTransaction rather than open the method over there in 488 // the regionserver package .
        Hide
        Ted Yu added a comment -

        When I ran TestCatalogTracker based on 5328v2.txt, I got:

        Failed tests:   testNoTimeoutWaitForRoot(org.apache.hadoop.hbase.catalog.TestCatalogTracker): Assert WaitOnMeta still waiting
        
        Show
        Ted Yu added a comment - When I ran TestCatalogTracker based on 5328v2.txt, I got: Failed tests: testNoTimeoutWaitForRoot(org.apache.hadoop.hbase.catalog.TestCatalogTracker): Assert WaitOnMeta still waiting
        Hide
        stack added a comment -

        Looking for reviewers. I posted to rb. This is stuff that will help make the master more testable including sample tests. Also fixes HBASE-5594 (with test to prove it does).

        Show
        stack added a comment - Looking for reviewers. I posted to rb. This is stuff that will help make the master more testable including sample tests. Also fixes HBASE-5594 (with test to prove it does).
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for hbase.

        Summary
        -------

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        Make this class public so its waitForRoot(long) can be used by HMaster.
        Remove the stalling waitForRoot no arg.
        M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        Javadoc. Add check if stopped flag cycling waiting on assignment.
        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        Moved check if schema change flag out to a method rather than
        have it on tail of constructor.
        Moved other initialization stuff like get of assignment manager
        and server manager out into methods so could be intercepted by
        tests and mocking.
        Change how we wait on root so we sleep 100ms at a time and always
        check stopped flag rather than block for ever.
        Added more checking if stopped flag.
        Added flag for when rpc server is up, mostly for tests.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
        Unused import.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        Comment.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
        Remove unused code.
        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        Change how we wait on root. DOn't use removed method.
        A src/test/java/org/apache/hadoop/hbase/master/MockRegionS

        This addresses bug hbase-5328.
        https://issues.apache.org/jira/browse/hbase-5328

        Diffs


        src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604
        src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a
        src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01
        src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb
        src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97
        src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a
        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION

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

        Testing
        -------

        Thanks,

        Michael

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4436/ ----------------------------------------------------------- Review request for hbase. Summary ------- M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot(long) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionS This addresses bug hbase-5328. https://issues.apache.org/jira/browse/hbase-5328 Diffs src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 79b6604 src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 90fa45a src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f97b01 src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java a929e31 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 0d7cbeb src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java beaff97 src/test/java/org/apache/hadoop/hbase/master/Mocking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java PRE-CREATION Diff: https://reviews.apache.org/r/4436/diff Testing ------- Thanks, Michael
        Hide
        stack added a comment -

        Patch looks fat but its mostly comments, javadoc, and new Tests and Test facility.

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
          Make this class public so its waitForRoot(long) can be used by HMaster.
          Remove the stalling waitForRoot no arg.
        M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
          Javadoc.  Add check if stopped flag cycling waiting on assignment.
        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          Moved check if schema change flag out to a method rather than
          have it on tail of constructor.
          Moved other initialization stuff like get of assignment manager
          and server manager out into methods so could be intercepted by
          tests and mocking.
          Change how we wait on root so we sleep 100ms at a time and always
          check stopped flag rather than block for ever.
          Added more checking if stopped flag.
          Added flag for when rpc server is up, mostly for tests.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
          Unused import.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
          Comment.
        M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
          Remove unused code.
        M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
          Change how we wait on root.  DOn't use removed method.
        A src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
          Added a mock regionserver used faking out master.  Later this may
          become more generally useable.  Can move it up out of this package 
          then but for now just used by TestMasterNoCluster.
        M src/test/java/org/apache/hadoop/hbase/master/Mocking.java
          Mocking utilities used only by a few of the classes in this package.
          Later may be generally useful.  Will move it then.
        M src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
          Use new Mocking utilities.  Move method from here out to Mocking.
        M src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
          Test stopping master on initialization as well as master failover and
          startup sequence.
        
        Show
        stack added a comment - Patch looks fat but its mostly comments, javadoc, and new Tests and Test facility. M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java Make this class public so its waitForRoot( long ) can be used by HMaster. Remove the stalling waitForRoot no arg. M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc. Add check if stopped flag cycling waiting on assignment. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Moved check if schema change flag out to a method rather than have it on tail of constructor. Moved other initialization stuff like get of assignment manager and server manager out into methods so could be intercepted by tests and mocking. Change how we wait on root so we sleep 100ms at a time and always check stopped flag rather than block for ever. Added more checking if stopped flag. Added flag for when rpc server is up, mostly for tests. M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java Unused import . M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Comment. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Remove unused code. M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java Change how we wait on root. DOn't use removed method. A src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java Added a mock regionserver used faking out master. Later this may become more generally useable. Can move it up out of this package then but for now just used by TestMasterNoCluster. M src/test/java/org/apache/hadoop/hbase/master/Mocking.java Mocking utilities used only by a few of the classes in this package . Later may be generally useful. Will move it then. M src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java Use new Mocking utilities. Move method from here out to Mocking. M src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java Test stopping master on initialization as well as master failover and startup sequence.
        Hide
        stack added a comment -

        I think MockRegionServer.java should be placed under src/test/java/org/apache/hadoop/hbase/regionserver/

        Not yet. Its built for testing the Master (as it says on the class comment). Later when it gets flushed out more it might make sense being moved elsewhere.

        Thanks for review. Hold off though till its more finished. Good on you Ted.

        Show
        stack added a comment - I think MockRegionServer.java should be placed under src/test/java/org/apache/hadoop/hbase/regionserver/ Not yet. Its built for testing the Master (as it says on the class comment). Later when it gets flushed out more it might make sense being moved elsewhere. Thanks for review. Hold off though till its more finished. Good on you Ted.
        Hide
        Ted Yu added a comment -

        I think MockRegionServer.java should be placed under src/test/java/org/apache/hadoop/hbase/regionserver/

        Show
        Ted Yu added a comment - I think MockRegionServer.java should be placed under src/test/java/org/apache/hadoop/hbase/regionserver/
        Hide
        stack added a comment -

        Need to finish more tests to prove these changes enough (also need to make it so you don't need to know that much about Master's workings writing tests).

        M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        (waitForRoot) Remove toxic method
        M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        Javadoc and added check of master still running in a few strategic
        places so we don't do infinite loop.
        M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        Refactor moving the instant schema change out to its own method.
        Added flag to indicate when master can receive RPCs.
        (createCatalogTracker, createServerManager, getRemoteInetAddress): put these creations
        out in methods so can override in tests to insert mocks, etc.
        A src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        Added a mock regionserver in the master package for testing master
        A src/test/java/org/apache/hadoop/hbase/master/Mocking.java
        Utility shared by tests in the master package
        A src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        A few tests that put up master and poke it to make it move
        through states.

        Patch is not done. Need to remove the gratuitous changes. Also need to add some tests that simulate some of the states that Ram and crew are finding.

        Show
        stack added a comment - Need to finish more tests to prove these changes enough (also need to make it so you don't need to know that much about Master's workings writing tests). M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java (waitForRoot) Remove toxic method M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Javadoc and added check of master still running in a few strategic places so we don't do infinite loop. M src/main/java/org/apache/hadoop/hbase/master/HMaster.java Refactor moving the instant schema change out to its own method. Added flag to indicate when master can receive RPCs. (createCatalogTracker, createServerManager, getRemoteInetAddress): put these creations out in methods so can override in tests to insert mocks, etc. A src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java Added a mock regionserver in the master package for testing master A src/test/java/org/apache/hadoop/hbase/master/Mocking.java Utility shared by tests in the master package A src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java A few tests that put up master and poke it to make it move through states. Patch is not done. Need to remove the gratuitous changes. Also need to add some tests that simulate some of the states that Ram and crew are finding.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development