HBase
  1. HBase
  2. HBASE-2993

Refactor servers to use a common lifecycle interface

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.90.0
    • Fix Version/s: None
    • Component/s: master, regionserver
    • Labels:
      None

      Description

      In current trunk, the region server is a Runnable and the Master is a thread. We have all kinds of weird wrappers like JVMClusterUtil to try to work around this. It would be nice if they both implemented the same interface - LocalHBaseCluster and the MiniCluster would be a lot easier to understand as well, and we could share some more code between them.

        Issue Links

          Activity

          Hide
          HBase Review Board added a comment -

          Message from: "Todd Lipcon" <todd@cloudera.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/829/
          -----------------------------------------------------------

          Review request for hbase, stack and Jonathan Gray.

          Summary
          -------

          This isn't really cleaned up, but wanted to gather opinions before I put in more effort. Does this kind of refactor seem good? Is the Guava "Service" class what we want to use or should we just write our own similar interface?

          This addresses bug HBASE-2993.
          http://issues.apache.org/jira/browse/HBASE-2993

          Diffs


          src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java bc0a62f
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java bc21a1e
          src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java c675db9
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java bba7b67
          src/main/java/org/apache/hadoop/hbase/service/ServiceUtils.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/service/ServiceWithMainThread.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java 4a9f1c3
          src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java b7abb51
          src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 9cc1168
          src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 9407c1e
          src/test/java/org/apache/hadoop/hbase/TestHBaseTestingUtility.java 1cd88d3
          src/test/java/org/apache/hadoop/hbase/TestInfoServers.java daffe02
          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 6908111
          src/test/java/org/apache/hadoop/hbase/master/TestKillingServersFromMaster.java f5fd243
          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java a172e2c
          src/test/java/org/apache/hadoop/hbase/regionserver/DisabledTestRegionServerExit.java 5b8b464
          src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 287f1fb

          Diff: http://review.cloudera.org/r/829/diff

          Testing
          -------

          Tried to run unit tests, but plenty failed. I think they're failing on trunk, too, though. Will do more testing and another round of review before I actually claim this is ready to commit.

          Thanks,

          Todd

          Show
          HBase Review Board added a comment - Message from: "Todd Lipcon" <todd@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/829/ ----------------------------------------------------------- Review request for hbase, stack and Jonathan Gray. Summary ------- This isn't really cleaned up, but wanted to gather opinions before I put in more effort. Does this kind of refactor seem good? Is the Guava "Service" class what we want to use or should we just write our own similar interface? This addresses bug HBASE-2993 . http://issues.apache.org/jira/browse/HBASE-2993 Diffs src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java bc0a62f src/main/java/org/apache/hadoop/hbase/master/HMaster.java bc21a1e src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java c675db9 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java bba7b67 src/main/java/org/apache/hadoop/hbase/service/ServiceUtils.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/service/ServiceWithMainThread.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java 4a9f1c3 src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java b7abb51 src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 9cc1168 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 9407c1e src/test/java/org/apache/hadoop/hbase/TestHBaseTestingUtility.java 1cd88d3 src/test/java/org/apache/hadoop/hbase/TestInfoServers.java daffe02 src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 6908111 src/test/java/org/apache/hadoop/hbase/master/TestKillingServersFromMaster.java f5fd243 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java a172e2c src/test/java/org/apache/hadoop/hbase/regionserver/DisabledTestRegionServerExit.java 5b8b464 src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 287f1fb Diff: http://review.cloudera.org/r/829/diff Testing ------- Tried to run unit tests, but plenty failed. I think they're failing on trunk, too, though. Will do more testing and another round of review before I actually claim this is ready to commit. Thanks, Todd
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/829/#review1173
          -----------------------------------------------------------

          I like this. Naming seems fine to me. We're making great strides in cleaning these things up, great stuff Todd.

          I guess we're in need of standardizing the methods in HM and HRS like toString, get*Name, etc...

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <http://review.cloudera.org/r/829/#comment4026>

          The toString() is supposed to give a unique name for the instance, especially useful in unit tests with multiple masters and such. Not sure where all we're using getServerName but might make sense for these to be the same.

          • Jonathan
          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/829/#review1173 ----------------------------------------------------------- I like this. Naming seems fine to me. We're making great strides in cleaning these things up, great stuff Todd. I guess we're in need of standardizing the methods in HM and HRS like toString, get*Name, etc... src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/829/#comment4026 > The toString() is supposed to give a unique name for the instance, especially useful in unit tests with multiple masters and such. Not sure where all we're using getServerName but might make sense for these to be the same. Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/829/#review1176
          -----------------------------------------------------------

          src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
          <http://review.cloudera.org/r/829/#comment4028>

          Didn't realize one of these lifecycle things in Guava but then there's a million different versions of Lifecycle I suppose (I'm pretty sure I've written a few myself in past life). We could write our own but, its my guess it'd not differ from what we have here much. Then again its missing Abort.

          This kinda refactoring is a big improvement; we should go this way, yeah, as it takes us further down road already started where we're trying to have Servers in hbase look alike. But I want us to be even more radical. I want us to move up to something like Spring where implementing their container means we get a bunch of other facility for near free and where the wiring up of a regionserver or a master can be done from config with others able to provide alternate implementations if they'd like with just a config. change. It'd encourage writing to Interfaces, etc. ( Well, maybe not Spring. Its too heavy weight and that xml stuff makes me queasy. There are lesser IofC containers such as pico or nano).

          src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
          <http://review.cloudera.org/r/829/#comment4029>

          This is good

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <http://review.cloudera.org/r/829/#comment4030>

          Thats a real pretty name (descriptive though I suppose)

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <http://review.cloudera.org/r/829/#comment4031>

          We had a sort of convention for naming threads in hbase where host 'server' is prefix where host includes service type and port if needed... that kinda thing. Previous we had a naming convention per implementator... was a mess.

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <http://review.cloudera.org/r/829/#comment4032>

          I like having state in there. I was also adding zk sessionid....

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          <http://review.cloudera.org/r/829/#comment4033>

          good

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          <http://review.cloudera.org/r/829/#comment4034>

          good

          src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java
          <http://review.cloudera.org/r/829/#comment4035>

          Good. I think I made this class originally. It grew into a monster.

          src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java
          <http://review.cloudera.org/r/829/#comment4036>

          Are we losing the threaddumping? It was useful.

          • stack
          Show
          HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/829/#review1176 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java < http://review.cloudera.org/r/829/#comment4028 > Didn't realize one of these lifecycle things in Guava but then there's a million different versions of Lifecycle I suppose (I'm pretty sure I've written a few myself in past life). We could write our own but, its my guess it'd not differ from what we have here much. Then again its missing Abort. This kinda refactoring is a big improvement; we should go this way, yeah, as it takes us further down road already started where we're trying to have Servers in hbase look alike. But I want us to be even more radical. I want us to move up to something like Spring where implementing their container means we get a bunch of other facility for near free and where the wiring up of a regionserver or a master can be done from config with others able to provide alternate implementations if they'd like with just a config. change. It'd encourage writing to Interfaces, etc. ( Well, maybe not Spring. Its too heavy weight and that xml stuff makes me queasy. There are lesser IofC containers such as pico or nano). src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java < http://review.cloudera.org/r/829/#comment4029 > This is good src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/829/#comment4030 > Thats a real pretty name (descriptive though I suppose) src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/829/#comment4031 > We had a sort of convention for naming threads in hbase where host 'server' is prefix where host includes service type and port if needed... that kinda thing. Previous we had a naming convention per implementator... was a mess. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < http://review.cloudera.org/r/829/#comment4032 > I like having state in there. I was also adding zk sessionid.... src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < http://review.cloudera.org/r/829/#comment4033 > good src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < http://review.cloudera.org/r/829/#comment4034 > good src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java < http://review.cloudera.org/r/829/#comment4035 > Good. I think I made this class originally. It grew into a monster. src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java < http://review.cloudera.org/r/829/#comment4036 > Are we losing the threaddumping? It was useful. stack

            People

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

              Dates

              • Created:
                Updated:

                Development