Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.95.2
    • Fix Version/s: 0.95.0
    • Component/s: Client, IPC/RPC, regionserver
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed

      Description

      As a step to move internals to PB, so as to avoid the conversion for performance reason, we should remove the HRegionInterface.
      Therefore region server only supports ClientProtocol and AdminProtocol. Later on, HRegion can work with PB messages directly.

      1. hbase_5889_v2.patch
        387 kB
        Jimmy Xiang
      2. hbase_5889_v4.patch
        389 kB
        Jimmy Xiang
      3. hbase_5889.patch
        388 kB
        Jimmy Xiang
      4. hbase-5889_v3.patch
        436 kB
        Jimmy Xiang

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Can you explain which layer of conversion you're trying to remove? I think we still need our own interface for clients – clients shouldn't directly speak protobuf.

          Rather than doing a big change like this, can we get some good profiling on a benchmark and see where our time is going? We can probably make up the lost time in other ways.

          Show
          Todd Lipcon added a comment - Can you explain which layer of conversion you're trying to remove? I think we still need our own interface for clients – clients shouldn't directly speak protobuf. Rather than doing a big change like this, can we get some good profiling on a benchmark and see where our time is going? We can probably make up the lost time in other ways.
          Hide
          Jimmy Xiang added a comment -

          The existing functions in the client side will not be touched for now. Probably we will add new methods to use the new types later.

          I was thinking to remove the conversion in server side, for example, HRegion. But for now, I'd like to remove HRegionInterface at first so
          as to clean up the references to it. This is necessary since I found some test codes are still using the old interface. It will be
          good for us to test the new interface.

          Show
          Jimmy Xiang added a comment - The existing functions in the client side will not be touched for now. Probably we will add new methods to use the new types later. I was thinking to remove the conversion in server side, for example, HRegion. But for now, I'd like to remove HRegionInterface at first so as to clean up the references to it. This is necessary since I found some test codes are still using the old interface. It will be good for us to test the new interface.
          Hide
          stack added a comment -

          @Jimmy You are trying to get rid of the conversion we currently do where if the server receives a pb Get, we have to change the http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.GetOrBuilder.html into a http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/client/Get.html so you can call http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/regionserver/HRegionServer.html#get(byte[], org.apache.hadoop.hbase.client.Get) ?

          Sounds good to me.

          Is there a prob. doing this Todd that you see?

          Show
          stack added a comment - @Jimmy You are trying to get rid of the conversion we currently do where if the server receives a pb Get, we have to change the http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.GetOrBuilder.html into a http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/client/Get.html so you can call http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/regionserver/HRegionServer.html#get(byte[ ], org.apache.hadoop.hbase.client.Get) ? Sounds good to me. Is there a prob. doing this Todd that you see?
          Hide
          Todd Lipcon added a comment -

          I'm just not sure this is actually the best bang for the buck, and might make layering less clean. I'm looking at RS CPU usage right now and the protobuf conversion doesn't show up at all in oprofile results. Finding a number of other hotspots for this kind of workload, though (see other JIRAs I'm filing)

          Show
          Todd Lipcon added a comment - I'm just not sure this is actually the best bang for the buck, and might make layering less clean. I'm looking at RS CPU usage right now and the protobuf conversion doesn't show up at all in oprofile results. Finding a number of other hotspots for this kind of workload, though (see other JIRAs I'm filing)
          Hide
          stack added a comment -

          I'm just not sure this is actually the best bang for the buck, and might make layering less clean.

          Because the HRegion APIs would all take pbs rather than the Get/Put/Delete, etc.? And doing this conversion would be a bunch of work that would be better spent doing other stuff?

          Serverside, going from pb into Get/Delete/Put just to get the data into and out of regions seems gratuitous and crud we should purge.

          Your profiling though would seem to make this a minor issue, one I would have thought prviously critical to address.

          Show
          stack added a comment - I'm just not sure this is actually the best bang for the buck, and might make layering less clean. Because the HRegion APIs would all take pbs rather than the Get/Put/Delete, etc.? And doing this conversion would be a bunch of work that would be better spent doing other stuff? Serverside, going from pb into Get/Delete/Put just to get the data into and out of regions seems gratuitous and crud we should purge. Your profiling though would seem to make this a minor issue, one I would have thought prviously critical to address.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for hbase.

          Summary
          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

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

          Diffs


          conf/hbase-policy.xml e45f23c
          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb
          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4
          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e
          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528
          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd
          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e
          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993
          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea
          src/main/protobuf/Admin.proto 2ad6fb0
          src/main/protobuf/RPC.proto 105fb3f
          src/main/resources/hbase-default.xml f54b345
          src/main/resources/hbase-webapps/master/table.jsp ca7310c
          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3
          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a
          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f
          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e
          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7
          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28
          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66
          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988
          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5
          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd
          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17
          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1
          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e
          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd
          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8
          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948
          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing
          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          Ted Yu added a comment -

          HRegionInterface is used by asynchbase:

                writeHBaseString(buf, "org.apache.hadoop.hbase.ipc.HRegionInterface");
              final String klass = "org.apache.hadoop.hbase.ipc.HRegionInterface";
          ./src/RegionClient.java
          

          Should we start a discussion on dev@hbase to get wider feedback about the roadmap for non-bundled (third-party) HBase client(s) ?

          Show
          Ted Yu added a comment - HRegionInterface is used by asynchbase: writeHBaseString(buf, "org.apache.hadoop.hbase.ipc.HRegionInterface" ); final String klass = "org.apache.hadoop.hbase.ipc.HRegionInterface" ; ./src/RegionClient.java Should we start a discussion on dev@hbase to get wider feedback about the roadmap for non-bundled (third-party) HBase client(s) ?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525477/hbase_5889.patch
          against trunk revision .

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

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

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +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 does not introduce any 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/1747//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1747//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1747//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/12525477/hbase_5889.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 60 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +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 does not introduce any 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/1747//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1747//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1747//console This message is automatically generated.
          Hide
          Jimmy Xiang added a comment -

          @Ted, I posted a message to dev@hbase as suggested. I think it is to their benefits to migrate as well.

          Show
          Jimmy Xiang added a comment - @Ted, I posted a message to dev@hbase as suggested. I think it is to their benefits to migrate as well.
          Hide
          stack added a comment -

          Benoît knows about the coming pb conversion and is good w/ it.

          Show
          stack added a comment - Benoît knows about the coming pb conversion and is good w/ it.
          Hide
          Jimmy Xiang added a comment -

          Trunk moved, new patch.

          Show
          Jimmy Xiang added a comment - Trunk moved, new patch.
          Hide
          Jimmy Xiang added a comment -

          @Stack, thanks, good to know.

          Show
          Jimmy Xiang added a comment - @Stack, thanks, good to know.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Excellent Jimmy. High-level, too much has been moved to protobufutils IMO. Below I highlight where we have gone too far. What you think? Otherwise, these is not much of substance to my comments below. I'd be up for committing this patch before it rots and addressing issues raised in a new jira if thats what you'd prefer.

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java
          <https://reviews.apache.org/r/4993/#comment16658>

          Anyone working on removal of HMasterInterface?

          Devaraj will need to pick up these changes if your patch goes in before his.

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
          <https://reviews.apache.org/r/4993/#comment16659>

          Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit?

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
          <https://reviews.apache.org/r/4993/#comment16660>

          ok

          src/main/java/org/apache/hadoop/hbase/HConstants.java
          <https://reviews.apache.org/r/4993/#comment16662>

          These are just not used?

          We used have a mechanism where you could subclass regionserver and then configure it so your subclass was started out on the cluster. You think that still possible?

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          <https://reviews.apache.org/r/4993/#comment16663>

          Hurray!

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          <https://reviews.apache.org/r/4993/#comment16664>

          Is it going too far adding methods like this into protobufutils? These methods seem pretty core hbase facility, too core to be out in a util method? You have them here because there is a bunch of pb'ing going on?

          I know I suggested that we move some of the transforms out here... but maybe this is taking it a bit far?

          What you reckon?

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
          <https://reviews.apache.org/r/4993/#comment16665>

          I think this kinda builder is a the right thing to have over in this util class.

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          <https://reviews.apache.org/r/4993/#comment16666>

          Like I said to Gregory last night, its kinda hard hiding this pb stuff when you are in the class that is slinging them.

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          <https://reviews.apache.org/r/4993/#comment16667>

          Is this new or moved code?

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
          <https://reviews.apache.org/r/4993/#comment16668>

          This was no longer a good idea?

          src/main/protobuf/Admin.proto
          <https://reviews.apache.org/r/4993/#comment16669>

          good

          src/main/resources/hbase-default.xml
          <https://reviews.apache.org/r/4993/#comment16670>

          good

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java
          <https://reviews.apache.org/r/4993/#comment16671>

          It is unexpected going to ProtobufUtils to get online regions.

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java
          <https://reviews.apache.org/r/4993/#comment16672>

          ditto

          • Michael

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7528 ----------------------------------------------------------- Excellent Jimmy. High-level, too much has been moved to protobufutils IMO. Below I highlight where we have gone too far. What you think? Otherwise, these is not much of substance to my comments below. I'd be up for committing this patch before it rots and addressing issues raised in a new jira if thats what you'd prefer. security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java < https://reviews.apache.org/r/4993/#comment16658 > Anyone working on removal of HMasterInterface? Devaraj will need to pick up these changes if your patch goes in before his. src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon < https://reviews.apache.org/r/4993/#comment16659 > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit? src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon < https://reviews.apache.org/r/4993/#comment16660 > ok src/main/java/org/apache/hadoop/hbase/HConstants.java < https://reviews.apache.org/r/4993/#comment16662 > These are just not used? We used have a mechanism where you could subclass regionserver and then configure it so your subclass was started out on the cluster. You think that still possible? src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java < https://reviews.apache.org/r/4993/#comment16663 > Hurray! src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java < https://reviews.apache.org/r/4993/#comment16664 > Is it going too far adding methods like this into protobufutils? These methods seem pretty core hbase facility, too core to be out in a util method? You have them here because there is a bunch of pb'ing going on? I know I suggested that we move some of the transforms out here... but maybe this is taking it a bit far? What you reckon? src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java < https://reviews.apache.org/r/4993/#comment16665 > I think this kinda builder is a the right thing to have over in this util class. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < https://reviews.apache.org/r/4993/#comment16666 > Like I said to Gregory last night, its kinda hard hiding this pb stuff when you are in the class that is slinging them. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < https://reviews.apache.org/r/4993/#comment16667 > Is this new or moved code? src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java < https://reviews.apache.org/r/4993/#comment16668 > This was no longer a good idea? src/main/protobuf/Admin.proto < https://reviews.apache.org/r/4993/#comment16669 > good src/main/resources/hbase-default.xml < https://reviews.apache.org/r/4993/#comment16670 > good src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java < https://reviews.apache.org/r/4993/#comment16671 > It is unexpected going to ProtobufUtils to get online regions. src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java < https://reviews.apache.org/r/4993/#comment16672 > ditto Michael On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java, line 38

          > <https://reviews.apache.org/r/4993/diff/1/?file=106380#file106380line38>

          >

          > Anyone working on removal of HMasterInterface?

          >

          > Devaraj will need to pick up these changes if your patch goes in before his.

          I'm working on the HMasterInterface. See HBASE-5445.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon, line 41

          > <https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41>

          >

          > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit?

          There isn't currently a RegionLoad that hides pb RegionLoad in the way you've described with ServerLoad. I actually wrote one up, but didn't include it because there is an existing RegionLoad that is needed until the HMasterInterface is done. I felt it made more sense to wait rather than spend a bunch of time renaming stuff.

          If this patch is committed as is, let's file a JIRA and assign it to me?

          • Gregory

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

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-03 22:57:57, Michael Stack wrote: > security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java, line 38 > < https://reviews.apache.org/r/4993/diff/1/?file=106380#file106380line38 > > > Anyone working on removal of HMasterInterface? > > Devaraj will need to pick up these changes if your patch goes in before his. I'm working on the HMasterInterface. See HBASE-5445 . On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon, line 41 > < https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41 > > > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit? There isn't currently a RegionLoad that hides pb RegionLoad in the way you've described with ServerLoad. I actually wrote one up, but didn't include it because there is an existing RegionLoad that is needed until the HMasterInterface is done. I felt it made more sense to wait rather than spend a bunch of time renaming stuff. If this patch is committed as is, let's file a JIRA and assign it to me? Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7528 ----------------------------------------------------------- On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon, line 41

          > <https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41>

          >

          > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit?

          Gregory Chanan wrote:

          There isn't currently a RegionLoad that hides pb RegionLoad in the way you've described with ServerLoad. I actually wrote one up, but didn't include it because there is an existing RegionLoad that is needed until the HMasterInterface is done. I felt it made more sense to wait rather than spend a bunch of time renaming stuff.

          If this patch is committed as is, let's file a JIRA and assign it to me?

          Grand!

          • Michael

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

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon, line 41 > < https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41 > > > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit? Gregory Chanan wrote: There isn't currently a RegionLoad that hides pb RegionLoad in the way you've described with ServerLoad. I actually wrote one up, but didn't include it because there is an existing RegionLoad that is needed until the HMasterInterface is done. I felt it made more sense to wait rather than spend a bunch of time renaming stuff. If this patch is committed as is, let's file a JIRA and assign it to me? Grand! Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7528 ----------------------------------------------------------- On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525521/hbase_5889_v2.patch
          against trunk revision .

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

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

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +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 does not introduce any 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/1753//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1753//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1753//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/12525521/hbase_5889_v2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 60 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +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 does not introduce any 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/1753//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1753//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1753//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon, line 41

          > <https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41>

          >

          > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit?

          Gregory Chanan wrote:

          There isn't currently a RegionLoad that hides pb RegionLoad in the way you've described with ServerLoad. I actually wrote one up, but didn't include it because there is an existing RegionLoad that is needed until the HMasterInterface is done. I felt it made more sense to wait rather than spend a bunch of time renaming stuff.

          If this patch is committed as is, let's file a JIRA and assign it to me?

          Michael Stack wrote:

          Grand!

          I rebased to trunk and got Gregory's patch.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 195

          > <https://reviews.apache.org/r/4993/diff/1/?file=106382#file106382line195>

          >

          > These are just not used?

          >

          > We used have a mechanism where you could subclass regionserver and then configure it so your subclass was started out on the cluster. You think that still possible?

          It is still possible. Now, if we want to subclass regionserver, we need to use HConstants.REGION_SERVER_IMPL. The HRegionInterface is not there any more.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, line 185

          > <https://reviews.apache.org/r/4993/diff/1/?file=106390#file106390line185>

          >

          > I think this kinda builder is a the right thing to have over in this util class.

          You are right. I am concerned with the size of the util class. So I'd like to have several special util classes.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 248

          > <https://reviews.apache.org/r/4993/diff/1/?file=106396#file106396line248>

          >

          > Is this new or moved code?

          It is moved from RegionServer, which is originally fro HRegionServer. Sorry for the confusion.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 1657

          > <https://reviews.apache.org/r/4993/diff/1/?file=106398#file106398line1657>

          >

          > This was no longer a good idea?

          It is hard to maintain two copies of implementation, especially one is not unit tested any more.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java, line 688

          > <https://reviews.apache.org/r/4993/diff/1/?file=106410#file106410line688>

          >

          > It is unexpected going to ProtobufUtils to get online regions.

          This method is used in many places. Should I put the util in HRegionServer as a static helper method?

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java, line 315

          > <https://reviews.apache.org/r/4993/diff/1/?file=106414#file106414line315>

          >

          > ditto

          How about put it in HRegionServer as a static helper util?

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1295

          > <https://reviews.apache.org/r/4993/diff/1/?file=106388#file106388line1295>

          >

          > Is it going too far adding methods like this into protobufutils? These methods seem pretty core hbase facility, too core to be out in a util method? You have them here because there is a bunch of pb'ing going on?

          >

          > I know I suggested that we move some of the transforms out here... but maybe this is taking it a bit far?

          >

          > What you reckon?

          openRegion is used in many test classes. That's why I have this in the util. Should I put in HRegionServer as a static helper util?

          • Jimmy

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

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon, line 41 > < https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41 > > > Is this right after Gregory's commit last night? He hides pb Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even after his commit? Gregory Chanan wrote: There isn't currently a RegionLoad that hides pb RegionLoad in the way you've described with ServerLoad. I actually wrote one up, but didn't include it because there is an existing RegionLoad that is needed until the HMasterInterface is done. I felt it made more sense to wait rather than spend a bunch of time renaming stuff. If this patch is committed as is, let's file a JIRA and assign it to me? Michael Stack wrote: Grand! I rebased to trunk and got Gregory's patch. On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 195 > < https://reviews.apache.org/r/4993/diff/1/?file=106382#file106382line195 > > > These are just not used? > > We used have a mechanism where you could subclass regionserver and then configure it so your subclass was started out on the cluster. You think that still possible? It is still possible. Now, if we want to subclass regionserver, we need to use HConstants.REGION_SERVER_IMPL. The HRegionInterface is not there any more. On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, line 185 > < https://reviews.apache.org/r/4993/diff/1/?file=106390#file106390line185 > > > I think this kinda builder is a the right thing to have over in this util class. You are right. I am concerned with the size of the util class. So I'd like to have several special util classes. On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 248 > < https://reviews.apache.org/r/4993/diff/1/?file=106396#file106396line248 > > > Is this new or moved code? It is moved from RegionServer, which is originally fro HRegionServer. Sorry for the confusion. On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 1657 > < https://reviews.apache.org/r/4993/diff/1/?file=106398#file106398line1657 > > > This was no longer a good idea? It is hard to maintain two copies of implementation, especially one is not unit tested any more. On 2012-05-03 22:57:57, Michael Stack wrote: > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java, line 688 > < https://reviews.apache.org/r/4993/diff/1/?file=106410#file106410line688 > > > It is unexpected going to ProtobufUtils to get online regions. This method is used in many places. Should I put the util in HRegionServer as a static helper method? On 2012-05-03 22:57:57, Michael Stack wrote: > src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java, line 315 > < https://reviews.apache.org/r/4993/diff/1/?file=106414#file106414line315 > > > ditto How about put it in HRegionServer as a static helper util? On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1295 > < https://reviews.apache.org/r/4993/diff/1/?file=106388#file106388line1295 > > > Is it going too far adding methods like this into protobufutils? These methods seem pretty core hbase facility, too core to be out in a util method? You have them here because there is a bunch of pb'ing going on? > > I know I suggested that we move some of the transforms out here... but maybe this is taking it a bit far? > > What you reckon? openRegion is used in many test classes. That's why I have this in the util. Should I put in HRegionServer as a static helper util? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7528 ----------------------------------------------------------- On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          <https://reviews.apache.org/r/4993/#comment16698>

          HRegionServer util might be better place for this yes. Or could they be put into a new class, RegionServerUtil in the regionserver package? Maybe they don't have to be public methods if done this way?

          Either sounds good to me boss.

          • Michael

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7539 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java < https://reviews.apache.org/r/4993/#comment16698 > HRegionServer util might be better place for this yes. Or could they be put into a new class, RegionServerUtil in the regionserver package? Maybe they don't have to be public methods if done this way? Either sounds good to me boss. Michael On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 1657

          > <https://reviews.apache.org/r/4993/diff/1/?file=106398#file106398line1657>

          >

          > This was no longer a good idea?

          Jimmy Xiang wrote:

          It is hard to maintain two copies of implementation, especially one is not unit tested any more.

          Ok.

          On 2012-05-03 22:57:57, Michael Stack wrote:

          > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java, line 688

          > <https://reviews.apache.org/r/4993/diff/1/?file=106410#file106410line688>

          >

          > It is unexpected going to ProtobufUtils to get online regions.

          Jimmy Xiang wrote:

          This method is used in many places. Should I put the util in HRegionServer as a static helper method?

          See above.

          In fact isn't onlineRegions a class of its own? If so, could be a static method in there?

          • Michael

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

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-03 22:57:57, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 1657 > < https://reviews.apache.org/r/4993/diff/1/?file=106398#file106398line1657 > > > This was no longer a good idea? Jimmy Xiang wrote: It is hard to maintain two copies of implementation, especially one is not unit tested any more. Ok. On 2012-05-03 22:57:57, Michael Stack wrote: > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java, line 688 > < https://reviews.apache.org/r/4993/diff/1/?file=106410#file106410line688 > > > It is unexpected going to ProtobufUtils to get online regions. Jimmy Xiang wrote: This method is used in many places. Should I put the util in HRegionServer as a static helper method? See above. In fact isn't onlineRegions a class of its own? If so, could be a static method in there? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7528 ----------------------------------------------------------- On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-04 01:08:06, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1295

          > <https://reviews.apache.org/r/4993/diff/1/?file=106388#file106388line1295>

          >

          > HRegionServer util might be better place for this yes. Or could they be put into a new class, RegionServerUtil in the regionserver package? Maybe they don't have to be public methods if done this way?

          >

          > Either sounds good to me boss.

          It is good to have a new class RegionServerUtil. Will do.

          • Jimmy

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

          On 2012-05-03 17:27:50, Jimmy Xiang wrote:

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

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

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

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

          (Updated 2012-05-03 17:27:50)

          Review request for hbase.

          Summary

          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

          This addresses bug HBASE-5889.

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

          Diffs

          -----

          conf/hbase-policy.xml e45f23c

          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb

          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4

          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e

          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528

          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd

          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993

          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e

          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea

          src/main/protobuf/Admin.proto 2ad6fb0

          src/main/protobuf/RPC.proto 105fb3f

          src/main/resources/hbase-default.xml f54b345

          src/main/resources/hbase-webapps/master/table.jsp ca7310c

          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3

          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a

          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f

          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e

          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28

          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66

          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988

          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5

          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd

          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17

          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1

          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e

          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8

          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948

          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing

          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-04 01:08:06, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1295 > < https://reviews.apache.org/r/4993/diff/1/?file=106388#file106388line1295 > > > HRegionServer util might be better place for this yes. Or could they be put into a new class, RegionServerUtil in the regionserver package? Maybe they don't have to be public methods if done this way? > > Either sounds good to me boss. It is good to have a new class RegionServerUtil. Will do. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/#review7539 ----------------------------------------------------------- On 2012-05-03 17:27:50, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-03 17:27:50) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs ----- conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java 0c4b4cb src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96ac8bd src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/main/resources/hbase-webapps/master/table.jsp ca7310c src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2012-05-04 18:16:37.788666)

          Review request for hbase.

          Changes
          -------

          Addressed Stack's comments.

          Summary
          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

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

          Diffs (updated)


          conf/hbase-policy.xml e45f23c
          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java fda40cc
          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4
          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
          src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 8a383e4
          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2
          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 11d8bf9
          src/main/java/org/apache/hadoop/hbase/client/HTable.java b8290e4
          src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 578b2b2
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e
          src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 9e4ada9
          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528
          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd
          src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 6ba8ab0
          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e
          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993
          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8c8381b
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerUtil.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 5050df0
          src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 7b4f4a2
          src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 9c3c9ef
          src/main/protobuf/Admin.proto 2ad6fb0
          src/main/protobuf/RPC.proto 105fb3f
          src/main/resources/hbase-default.xml f54b345
          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3
          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a
          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f
          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e
          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7
          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28
          src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 0079b13
          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66
          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988
          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5
          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd
          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17
          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1
          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e
          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd
          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8
          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948
          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing
          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-04 18:16:37.788666) Review request for hbase. Changes ------- Addressed Stack's comments. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs (updated) conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java fda40cc src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 8a383e4 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 11d8bf9 src/main/java/org/apache/hadoop/hbase/client/HTable.java b8290e4 src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 578b2b2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 9e4ada9 src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 6ba8ab0 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8c8381b src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerUtil.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 5050df0 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 7b4f4a2 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 9c3c9ef src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 0079b13 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525658/hbase-5889_v3.patch
          against trunk revision .

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

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

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +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 does not introduce any 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.replication.TestReplication

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1769//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1769//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1769//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/12525658/hbase-5889_v3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 63 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +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 does not introduce any 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.replication.TestReplication Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1769//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1769//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1769//console This message is automatically generated.
          Hide
          stack added a comment -

          Is this good to go in Jimmy? What comments did you address?

          Show
          stack added a comment - Is this good to go in Jimmy? What comments did you address?
          Hide
          Jimmy Xiang added a comment -

          Yes, I think it is good to go. I moved those utils like openRegion from ProtobufUtil to RegionServerUtil.

          Show
          Jimmy Xiang added a comment - Yes, I think it is good to go. I moved those utils like openRegion from ProtobufUtil to RegionServerUtil.
          Hide
          stack added a comment -

          I took a look. Its good. I tried changing RegionServerUtil so it was package private but I got these failures which are odd given the class is named RegionServerUtil. What you reckon?

          [INFO] Compilation failure
          
          /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java:[43,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package
          
          /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java:[79,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package
          
          /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java:[72,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package
          
          /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java:[70,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package
          
          /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java:[55,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package
          

          What are these classes making use of RegionServerUtil?

          Show
          stack added a comment - I took a look. Its good. I tried changing RegionServerUtil so it was package private but I got these failures which are odd given the class is named RegionServerUtil. What you reckon? [INFO] Compilation failure /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java:[43,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java:[79,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java:[72,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java:[70,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package /Users/Stack/checkouts/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java:[55,43] org.apache.hadoop.hbase.regionserver.RegionServerUtil is not public in org.apache.hadoop.hbase.regionserver; cannot be accessed from outside package What are these classes making use of RegionServerUtil?
          Hide
          Jimmy Xiang added a comment -

          The purpose is to re-use some shared code. I was thinking to separate it into two util classes: ClientUtil and AdminUtil, in the client package.
          It is not for region server only. How is that?

          Show
          Jimmy Xiang added a comment - The purpose is to re-use some shared code. I was thinking to separate it into two util classes: ClientUtil and AdminUtil, in the client package. It is not for region server only. How is that?
          Hide
          stack added a comment -

          The util is for serverside though? If not, if for client side, then yes, belongs in client package. Good on you Jimmy.

          Show
          stack added a comment - The util is for serverside though? If not, if for client side, then yes, belongs in client package. Good on you Jimmy.
          Hide
          Jimmy Xiang added a comment -

          After a second thought, I used protobuf util instead since it is for the client side. For the sever side case you referred to (openRegion returns a OpenRegionState), I moved it out of the util.

          Show
          Jimmy Xiang added a comment - After a second thought, I used protobuf util instead since it is for the client side. For the sever side case you referred to (openRegion returns a OpenRegionState), I moved it out of the util.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2012-05-04 22:54:46.398933)

          Review request for hbase.

          Summary
          -------

          Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer.

          The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side.

          Fixed some unit tests. Now all region server unit tests test the new pb functions.

          Enhanced getServerInfo so that it returns the webui port too.

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

          Diffs (updated)


          conf/hbase-policy.xml e45f23c
          security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java fda40cc
          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4
          src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e
          src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528
          src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd
          src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 6ba8ab0
          src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e
          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993
          src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8c8381b
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea
          src/main/protobuf/Admin.proto 2ad6fb0
          src/main/protobuf/RPC.proto 105fb3f
          src/main/resources/hbase-default.xml f54b345
          src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3
          src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a
          src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f
          src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e
          src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7
          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28
          src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66
          src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988
          src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5
          src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd
          src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17
          src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1
          src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e
          src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd
          src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8
          src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948
          src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7

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

          Testing
          -------

          All regular and security profile tests are green before I rebased to the latest today.

          Thanks,

          Jimmy

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4993/ ----------------------------------------------------------- (Updated 2012-05-04 22:54:46.398933) Review request for hbase. Summary ------- Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code from RegionServer back to HRegionServer. The goal is to avoid two copies of region server code to maintain, and make it possible to avoid data type conversion in the sever side. Fixed some unit tests. Now all region server unit tests test the new pb functions. Enhanced getServerInfo so that it returns the webui port too. This addresses bug HBASE-5889 . https://issues.apache.org/jira/browse/HBASE-5889 Diffs (updated) conf/hbase-policy.xml e45f23c security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java fda40cc src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 87f04f4 src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java cd9b528 src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 6ba8ab0 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java d1e0993 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 81603af src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java fbf0127 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java db1333b src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java ae2094d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 8b45f03 src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 827fb23 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8c8381b src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 4cb070e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java c2c89ea src/main/protobuf/Admin.proto 2ad6fb0 src/main/protobuf/RPC.proto 105fb3f src/main/resources/hbase-default.xml f54b345 src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3 src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java ad77e0a src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 3dfc94e src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 42092b7 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28 src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java c36272f src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java bdec3ee src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 7dbba66 src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 3acb988 src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java eb546a5 src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java ceba5cd src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java ec08b17 src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java 30c6cf1 src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java 8c3f67e src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java 7bfe4cd src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java ffce7e8 src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java aa5ca37 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 65fa948 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7 Diff: https://reviews.apache.org/r/4993/diff Testing ------- All regular and security profile tests are green before I rebased to the latest today. Thanks, Jimmy
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525693/hbase_5889_v4.patch
          against trunk revision .

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

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

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +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 does not introduce any 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/1777//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1777//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1777//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/12525693/hbase_5889_v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 60 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +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 does not introduce any 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/1777//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1777//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1777//console This message is automatically generated.
          Hide
          stack added a comment -

          I can't tell highlevel what you've done Jimmy? I see RegionServerUtil is missing. Where do you spread its content? I don't see obvious new util classes. Thanks boss.

          Show
          stack added a comment - I can't tell highlevel what you've done Jimmy? I see RegionServerUtil is missing. Where do you spread its content? I don't see obvious new util classes. Thanks boss.
          Hide
          Jimmy Xiang added a comment -

          I thought to create new util class for client side. I used protobuf util instead since it is for the client side. For the sever side case you referred to (openRegion returns a OpenRegionState), I moved it out of the util to the original place since it's not shared. I changed the shared one so that it doesn't refer to region server directly.

          Show
          Jimmy Xiang added a comment - I thought to create new util class for client side. I used protobuf util instead since it is for the client side. For the sever side case you referred to (openRegion returns a OpenRegionState), I moved it out of the util to the original place since it's not shared. I changed the shared one so that it doesn't refer to region server directly.
          Hide
          stack added a comment -

          I think this not the best soln. but I'm not going to argue for a better patch in here when this one is already fat and its gotten a +1 from hadoopqa. Lets get this in and work on polish in other issues. Good stuff Jimmy.

          Show
          stack added a comment - I think this not the best soln. but I'm not going to argue for a better patch in here when this one is already fat and its gotten a +1 from hadoopqa. Lets get this in and work on polish in other issues. Good stuff Jimmy.
          Hide
          stack added a comment -

          Committed TRUNK. Good stuff Jimmy.

          Show
          stack added a comment - Committed TRUNK. Good stuff Jimmy.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #192 (See https://builds.apache.org/job/HBase-TRUNK-security/192/)
          HBASE-5889 Remove HRegionInterface (Revision 1334314)

          Result = SUCCESS
          stack :
          Files :

          • /hbase/trunk/conf/hbase-policy.xml
          • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java
          • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
          • /hbase/trunk/src/main/protobuf/Admin.proto
          • /hbase/trunk/src/main/protobuf/RPC.proto
          • /hbase/trunk/src/main/resources/hbase-default.xml
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #192 (See https://builds.apache.org/job/HBase-TRUNK-security/192/ ) HBASE-5889 Remove HRegionInterface (Revision 1334314) Result = SUCCESS stack : Files : /hbase/trunk/conf/hbase-policy.xml /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/protobuf/Admin.proto /hbase/trunk/src/main/protobuf/RPC.proto /hbase/trunk/src/main/resources/hbase-default.xml /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2850 (See https://builds.apache.org/job/HBase-TRUNK/2850/)
          HBASE-5889 Remove HRegionInterface (Revision 1334314)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/conf/hbase-policy.xml
          • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java
          • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
          • /hbase/trunk/src/main/protobuf/Admin.proto
          • /hbase/trunk/src/main/protobuf/RPC.proto
          • /hbase/trunk/src/main/resources/hbase-default.xml
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2850 (See https://builds.apache.org/job/HBase-TRUNK/2850/ ) HBASE-5889 Remove HRegionInterface (Revision 1334314) Result = FAILURE stack : Files : /hbase/trunk/conf/hbase-policy.xml /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/protobuf/Admin.proto /hbase/trunk/src/main/protobuf/RPC.proto /hbase/trunk/src/main/resources/hbase-default.xml /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development