Details

    • Hadoop Flags:
      Reviewed
    1. hbase-5621_v3.patch
      303 kB
      Jimmy Xiang
    2. hbase_5621_v5.patch
      307 kB
      Jimmy Xiang
    3. hbase_5621_v4.patch
      306 kB
      Jimmy Xiang
    4. hbase_5621_v4.patch
      306 kB
      stack

      Activity

      Hide
      jiraposter@reviews.apache.org added a comment -

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

      Review request for hbase.

      Summary
      -------

      This is the admin part of HBase-5443. AdminProtocol part.

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

      Diffs


      src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION
      src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION
      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79
      src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72
      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a
      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9
      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf
      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e
      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f
      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58
      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc
      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2
      src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7
      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe
      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865
      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a
      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830
      src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3
      src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d
      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e
      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c
      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6
      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128
      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556
      src/main/protobuf/Admin.proto 132c5dd
      src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2
      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b
      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91
      src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b
      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de
      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e
      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27
      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152
      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115
      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e
      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15
      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c
      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45
      src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d

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

      Testing
      -------

      All unit tests passed.

      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/4714/ ----------------------------------------------------------- Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556 src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. 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/4714/#review6922
      -----------------------------------------------------------

      A few questions below... Looks great.

      src/main/java/org/apache/hadoop/hbase/AdminProtocol.java
      <https://reviews.apache.org/r/4714/#comment15405>

      Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile)

      This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level?

      src/main/java/org/apache/hadoop/hbase/ClientProtocol.java
      <https://reviews.apache.org/r/4714/#comment15406>

      Ditto

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

      Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize.

      I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there?

      src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
      <https://reviews.apache.org/r/4714/#comment15408>

      Great stuff.

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java
      <https://reviews.apache.org/r/4714/#comment15409>

      Did you say you were going to remove these from here?

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java
      <https://reviews.apache.org/r/4714/#comment15410>

      Yeah, does this belong in here?

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java
      <https://reviews.apache.org/r/4714/#comment15411>

      ditto

      An AdminProtocol should have an HConnection?

      Even if you moved this up into HConnectionManager for now... that'd be better.

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
      <https://reviews.apache.org/r/4714/#comment15412>

      Yeah, I wonder if this getClient and getAdmin should be @Overrides? Shoudl they be methods of HCM altogether? Would that be better? OR should it happen elsewhere? In HBaseAdmin or in HTable where we put together protocol and connection?

      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
      <https://reviews.apache.org/r/4714/#comment15413>

      Hurray!

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

      Where has allt his code gone?

      • Michael

      On 2012-04-13 23:03:35, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-13 23:03:35)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e

      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2

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

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

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

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

      Testing

      -------

      All unit tests passed.

      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/4714/#review6922 ----------------------------------------------------------- A few questions below... Looks great. src/main/java/org/apache/hadoop/hbase/AdminProtocol.java < https://reviews.apache.org/r/4714/#comment15405 > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile) This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level? src/main/java/org/apache/hadoop/hbase/ClientProtocol.java < https://reviews.apache.org/r/4714/#comment15406 > Ditto src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java < https://reviews.apache.org/r/4714/#comment15407 > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize. I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there? src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java < https://reviews.apache.org/r/4714/#comment15408 > Great stuff. src/main/java/org/apache/hadoop/hbase/client/HConnection.java < https://reviews.apache.org/r/4714/#comment15409 > Did you say you were going to remove these from here? src/main/java/org/apache/hadoop/hbase/client/HConnection.java < https://reviews.apache.org/r/4714/#comment15410 > Yeah, does this belong in here? src/main/java/org/apache/hadoop/hbase/client/HConnection.java < https://reviews.apache.org/r/4714/#comment15411 > ditto An AdminProtocol should have an HConnection? Even if you moved this up into HConnectionManager for now... that'd be better. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < https://reviews.apache.org/r/4714/#comment15412 > Yeah, I wonder if this getClient and getAdmin should be @Overrides? Shoudl they be methods of HCM altogether? Would that be better? OR should it happen elsewhere? In HBaseAdmin or in HTable where we put together protocol and connection? src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java < https://reviews.apache.org/r/4714/#comment15413 > Hurray! src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < https://reviews.apache.org/r/4714/#comment15414 > Where has allt his code gone? Michael On 2012-04-13 23:03:35, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-13 23:03:35) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556 src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. Thanks, Jimmy
      Hide
      jiraposter@reviews.apache.org added a comment -

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28

      > <https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28>

      >

      > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile)

      >

      > This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level?

      Let me move it to client.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29

      > <https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29>

      >

      > Ditto

      Will move to client.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632

      > <https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632>

      >

      > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize.

      >

      > I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there?

      For getRegionInfo, I used to put it in the protobuf util since it is used lots of places. I will move it to protobuf util.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32

      > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32>

      >

      > Did you say you were going to remove these from here?

      I tried but it is hard. Let me think about it again.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 223

      > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line223>

      >

      > Yeah, does this belong in here?

      Will fix it.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 260

      > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line260>

      >

      > ditto

      >

      > An AdminProtocol should have an HConnection?

      >

      > Even if you moved this up into HConnectionManager for now... that'd be better.

      Let me think about it and fix it.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1376

      > <https://reviews.apache.org/r/4714/diff/1/?file=101785#file101785line1376>

      >

      > Yeah, I wonder if this getClient and getAdmin should be @Overrides? Shoudl they be methods of HCM altogether? Would that be better? OR should it happen elsewhere? In HBaseAdmin or in HTable where we put together protocol and connection?

      Let me think about it and fix it.

      On 2012-04-13 23:59:40, Michael Stack wrote:

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

      > <https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814>

      >

      > Where has allt his code gone?

      Some are moved to RegionServer as I did for 5620

      • Jimmy

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

      On 2012-04-13 23:03:35, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-13 23:03:35)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e

      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2

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

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

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

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

      Testing

      -------

      All unit tests passed.

      Thanks,

      Jimmy

      Show
      jiraposter@reviews.apache.org added a comment - On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28 > < https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28 > > > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile) > > This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level? Let me move it to client. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29 > < https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29 > > > Ditto Will move to client. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632 > < https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632 > > > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize. > > I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there? For getRegionInfo, I used to put it in the protobuf util since it is used lots of places. I will move it to protobuf util. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32 > < https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32 > > > Did you say you were going to remove these from here? I tried but it is hard. Let me think about it again. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 223 > < https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line223 > > > Yeah, does this belong in here? Will fix it. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 260 > < https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line260 > > > ditto > > An AdminProtocol should have an HConnection? > > Even if you moved this up into HConnectionManager for now... that'd be better. Let me think about it and fix it. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1376 > < https://reviews.apache.org/r/4714/diff/1/?file=101785#file101785line1376 > > > Yeah, I wonder if this getClient and getAdmin should be @Overrides? Shoudl they be methods of HCM altogether? Would that be better? OR should it happen elsewhere? In HBaseAdmin or in HTable where we put together protocol and connection? Let me think about it and fix it. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 2814 > < https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814 > > > Where has allt his code gone? Some are moved to RegionServer as I did for 5620 Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/#review6922 ----------------------------------------------------------- On 2012-04-13 23:03:35, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-13 23:03:35) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556 src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. Thanks, Jimmy
      Hide
      jiraposter@reviews.apache.org added a comment -

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28

      > <https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28>

      >

      > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile)

      >

      > This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level?

      Jimmy Xiang wrote:

      Let me move it to client.

      And it should not be public I'd say.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29

      > <https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29>

      >

      > Ditto

      Jimmy Xiang wrote:

      Will move to client.

      Is this only used out of the client package? If so, yeah, move it there I'd say.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632

      > <https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632>

      >

      > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize.

      >

      > I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there?

      Jimmy Xiang wrote:

      For getRegionInfo, I used to put it in the protobuf util since it is used lots of places. I will move it to protobuf util.

      Agree. If used more than once, move it over to protobuf util otherwise I don't see a way around not importing protobuf classes when its a particular usage.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32

      > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32>

      >

      > Did you say you were going to remove these from here?

      Jimmy Xiang wrote:

      I tried but it is hard. Let me think about it again.

      Even if this stuff just moved to HCM, it'd give us a clean HConnection.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 260

      > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line260>

      >

      > ditto

      >

      > An AdminProtocol should have an HConnection?

      >

      > Even if you moved this up into HConnectionManager for now... that'd be better.

      Jimmy Xiang wrote:

      Let me think about it and fix it.

      Thanks.

      On 2012-04-13 23:59:40, Michael Stack wrote:

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

      > <https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814>

      >

      > Where has allt his code gone?

      Jimmy Xiang wrote:

      Some are moved to RegionServer as I did for 5620

      But I did not see it in the patch? Its already moved?

      • Michael

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

      On 2012-04-13 23:03:35, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-13 23:03:35)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e

      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2

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

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

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

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

      Testing

      -------

      All unit tests passed.

      Thanks,

      Jimmy

      Show
      jiraposter@reviews.apache.org added a comment - On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28 > < https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28 > > > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile) > > This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level? Jimmy Xiang wrote: Let me move it to client. And it should not be public I'd say. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29 > < https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29 > > > Ditto Jimmy Xiang wrote: Will move to client. Is this only used out of the client package? If so, yeah, move it there I'd say. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632 > < https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632 > > > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize. > > I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there? Jimmy Xiang wrote: For getRegionInfo, I used to put it in the protobuf util since it is used lots of places. I will move it to protobuf util. Agree. If used more than once, move it over to protobuf util otherwise I don't see a way around not importing protobuf classes when its a particular usage. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32 > < https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32 > > > Did you say you were going to remove these from here? Jimmy Xiang wrote: I tried but it is hard. Let me think about it again. Even if this stuff just moved to HCM, it'd give us a clean HConnection. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 260 > < https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line260 > > > ditto > > An AdminProtocol should have an HConnection? > > Even if you moved this up into HConnectionManager for now... that'd be better. Jimmy Xiang wrote: Let me think about it and fix it. Thanks. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 2814 > < https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814 > > > Where has allt his code gone? Jimmy Xiang wrote: Some are moved to RegionServer as I did for 5620 But I did not see it in the patch? Its already moved? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/#review6922 ----------------------------------------------------------- On 2012-04-13 23:03:35, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-13 23:03:35) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556 src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. Thanks, Jimmy
      Hide
      jiraposter@reviews.apache.org added a comment -

      On 2012-04-13 23:59:40, Michael Stack wrote:

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

      > <https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814>

      >

      > Where has allt his code gone?

      Jimmy Xiang wrote:

      Some are moved to RegionServer as I did for 5620

      Michael Stack wrote:

      But I did not see it in the patch? Its already moved?

      It should be in the patch, probably in the second page of touched files.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29

      > <https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29>

      >

      > Ditto

      Jimmy Xiang wrote:

      Will move to client.

      Michael Stack wrote:

      Is this only used out of the client package? If so, yeah, move it there I'd say.

      It is used out of the client package, and the server side implements it.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28

      > <https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28>

      >

      > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile)

      >

      > This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level?

      Jimmy Xiang wrote:

      Let me move it to client.

      Michael Stack wrote:

      And it should not be public I'd say.

      Yes, it is private.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632

      > <https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632>

      >

      > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize.

      >

      > I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there?

      Jimmy Xiang wrote:

      For getRegionInfo, I used to put it in the protobuf util since it is used lots of places. I will move it to protobuf util.

      Michael Stack wrote:

      Agree. If used more than once, move it over to protobuf util otherwise I don't see a way around not importing protobuf classes when its a particular usage.

      Cool, will fix.

      On 2012-04-13 23:59:40, Michael Stack wrote:

      > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32

      > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32>

      >

      > Did you say you were going to remove these from here?

      Jimmy Xiang wrote:

      I tried but it is hard. Let me think about it again.

      Michael Stack wrote:

      Even if this stuff just moved to HCM, it'd give us a clean HConnection.

      I was thinking to have a ProtocolFactory/Manager to manage the protocols, and let the connection manager to many connections only.
      Let me move them to HCM for now.

      • Jimmy

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

      On 2012-04-13 23:03:35, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-13 23:03:35)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e

      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2

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

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

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

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

      Testing

      -------

      All unit tests passed.

      Thanks,

      Jimmy

      Show
      jiraposter@reviews.apache.org added a comment - On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 2814 > < https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814 > > > Where has allt his code gone? Jimmy Xiang wrote: Some are moved to RegionServer as I did for 5620 Michael Stack wrote: But I did not see it in the patch? Its already moved? It should be in the patch, probably in the second page of touched files. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29 > < https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29 > > > Ditto Jimmy Xiang wrote: Will move to client. Michael Stack wrote: Is this only used out of the client package? If so, yeah, move it there I'd say. It is used out of the client package, and the server side implements it. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28 > < https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28 > > > Is this right? It does more than talk to a regionserver? You have to have really nice comments on your class now that you are at the top level Jimmy (smile) > > This class is only used by HBaseAdmin? Is that right? Or do other classes use it? If only HBaseAdmin, maybe it should not be top level? Jimmy Xiang wrote: Let me move it to client. Michael Stack wrote: And it should not be public I'd say. Yes, it is private. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632 > < https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632 > > > Do we have to have protobuf stuff sprinkled all over the code base? Its not too bad but just wondering. Maybe we do but just wondering. I suppose there is nothing to do about it. If we did same operation over and over w/ some pb stuff and the output was a non-pb object, then yes, we could hide the pb stuff over in a class but what we do here is particular to this method.... Can't generalize. > > I do see that you have added some static methods into protobuf utils. This one is not generic so doesn't deserve to go there? Jimmy Xiang wrote: For getRegionInfo, I used to put it in the protobuf util since it is used lots of places. I will move it to protobuf util. Michael Stack wrote: Agree. If used more than once, move it over to protobuf util otherwise I don't see a way around not importing protobuf classes when its a particular usage. Cool, will fix. On 2012-04-13 23:59:40, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32 > < https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32 > > > Did you say you were going to remove these from here? Jimmy Xiang wrote: I tried but it is hard. Let me think about it again. Michael Stack wrote: Even if this stuff just moved to HCM, it'd give us a clean HConnection. I was thinking to have a ProtocolFactory/Manager to manage the protocols, and let the connection manager to many connections only. Let me move them to HCM for now. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/#review6922 ----------------------------------------------------------- On 2012-04-13 23:03:35, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-13 23:03:35) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556 src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. 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/4714/
      -----------------------------------------------------------

      (Updated 2012-04-16 20:49:59.450802)

      Review request for hbase.

      Changes
      -------

      Addressed some of Stack's comments: 1. moved Client/AdminProtcol.java to client package, 2. moved some sharable pb calls to ProtobufUtil to avoid scattering pb everywhere.

      As to move some calls from HConnection to HCM, it is not as straightforward as I think. The main reason is becuase HConnection is not just a network connection to a server.
      Probably it is better to handle it in a separate jira.

      Summary
      -------

      This is the admin part of HBase-5443. AdminProtocol part.

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

      Diffs (updated)


      security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java 8219bea
      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79
      src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION
      src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION
      src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72
      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a
      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9
      src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50
      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf
      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e
      src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java d71e97e
      src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f
      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58
      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc
      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2
      src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9
      src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7
      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe
      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865
      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a
      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830
      src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3
      src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe
      src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d
      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1c0541f
      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d
      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c
      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6
      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128
      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c
      src/main/protobuf/Admin.proto 132c5dd
      src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2
      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b
      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91
      src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b
      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de
      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e
      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27
      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152
      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115
      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e
      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15
      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c
      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45
      src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10

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

      Testing
      -------

      All unit tests passed.

      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/4714/ ----------------------------------------------------------- (Updated 2012-04-16 20:49:59.450802) Review request for hbase. Changes ------- Addressed some of Stack's comments: 1. moved Client/AdminProtcol.java to client package, 2. moved some sharable pb calls to ProtobufUtil to avoid scattering pb everywhere. As to move some calls from HConnection to HCM, it is not as straightforward as I think. The main reason is becuase HConnection is not just a network connection to a server. Probably it is better to handle it in a separate jira. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs (updated) security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java 8219bea src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50 src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java d71e97e src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1c0541f src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10 Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. 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/4714/
      -----------------------------------------------------------

      (Updated 2012-04-19 17:46:17.775148)

      Review request for hbase.

      Changes
      -------

      Rebased to the latest of the trunk.

      Summary
      -------

      This is the admin part of HBase-5443. AdminProtocol part.

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

      Diffs (updated)


      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79
      src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION
      src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION
      src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72
      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a
      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9
      src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50
      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb
      src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15
      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443
      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d
      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8
      src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9
      src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a
      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe
      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865
      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a
      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830
      src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3
      src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe
      src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d
      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988
      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d
      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995
      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6
      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2
      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c
      src/main/protobuf/Admin.proto 132c5dd
      src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2
      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b
      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91
      src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b
      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de
      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e
      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27
      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152
      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd
      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e
      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15
      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c
      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45
      src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10

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

      Testing
      -------

      All unit tests passed.

      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/4714/ ----------------------------------------------------------- (Updated 2012-04-19 17:46:17.775148) Review request for hbase. Changes ------- Rebased to the latest of the trunk. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs (updated) src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50 src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8 src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995 src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10 Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. 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/4714/#review7046
      -----------------------------------------------------------

      security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java
      <https://reviews.apache.org/r/4714/#comment15632>

      There are a bunch of import changes here. Are they all needed?

      security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java
      <https://reviews.apache.org/r/4714/#comment15631>

      Why can we get away w/ removing the try/catch? Because the caller handles it?

      src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
      <https://reviews.apache.org/r/4714/#comment15633>

      good

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

      Does this belong in this patch? Is it part of another patch?

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

      Yeah, this stuff is from another patch? Why you adding it?

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

      Are these from Elliotts' patch?

      Maybe its reviewboard that is messing up? I'm only looking at diff between your v2 and v3 patch.

      • Michael

      On 2012-04-19 17:46:17, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-19 17:46:17)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

      src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb

      src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8

      src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9

      src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

      src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

      src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10

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

      Testing

      -------

      All unit tests passed.

      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/4714/#review7046 ----------------------------------------------------------- security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java < https://reviews.apache.org/r/4714/#comment15632 > There are a bunch of import changes here. Are they all needed? security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java < https://reviews.apache.org/r/4714/#comment15631 > Why can we get away w/ removing the try/catch? Because the caller handles it? src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java < https://reviews.apache.org/r/4714/#comment15633 > good src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4714/#comment15634 > Does this belong in this patch? Is it part of another patch? src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4714/#comment15635 > Yeah, this stuff is from another patch? Why you adding it? src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < https://reviews.apache.org/r/4714/#comment15636 > Are these from Elliotts' patch? Maybe its reviewboard that is messing up? I'm only looking at diff between your v2 and v3 patch. Michael On 2012-04-19 17:46:17, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-19 17:46:17) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50 src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8 src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995 src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10 Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. Thanks, Jimmy
      Hide
      jiraposter@reviews.apache.org added a comment -

      On 2012-04-19 20:43:31, Michael Stack wrote:

      >

      I did a rebase. So it picked up some other changes if you diff 2 from 3.

      On 2012-04-19 20:43:31, Michael Stack wrote:

      > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java, line 50

      > <https://reviews.apache.org/r/4714/diff/2/?file=102179#file102179line50>

      >

      > There are a bunch of import changes here. Are they all needed?

      It must be from other patch. This patch doesn't touch this file.

      On 2012-04-19 20:43:31, Michael Stack wrote:

      > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java, line 187

      > <https://reviews.apache.org/r/4714/diff/2/?file=102179#file102179line187>

      >

      > Why can we get away w/ removing the try/catch? Because the caller handles it?

      That change in diff2 is already checked in as part of 5443 addendum. That's why it shows some change here. This patch doesn't change this file.

      On 2012-04-19 20:43:31, Michael Stack wrote:

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

      > <https://reviews.apache.org/r/4714/diff/2-3/?file=102195#file102195line411>

      >

      > Does this belong in this patch? Is it part of another patch?

      From another patch due to rebasing.

      On 2012-04-19 20:43:31, Michael Stack wrote:

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

      > <https://reviews.apache.org/r/4714/diff/2-3/?file=102195#file102195line2220>

      >

      > Yeah, this stuff is from another patch? Why you adding it?

      From another patch due to rebasing.

      On 2012-04-19 20:43:31, Michael Stack wrote:

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

      > <https://reviews.apache.org/r/4714/diff/2-3/?file=102203#file102203line2214>

      >

      > Are these from Elliotts' patch?

      >

      > Maybe its reviewboard that is messing up? I'm only looking at diff between your v2 and v3 patch.

      From another patch due to rebasing.

      • Jimmy

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

      On 2012-04-19 17:46:17, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-19 17:46:17)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

      src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb

      src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8

      src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9

      src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

      src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

      src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10

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

      Testing

      -------

      All unit tests passed.

      Thanks,

      Jimmy

      Show
      jiraposter@reviews.apache.org added a comment - On 2012-04-19 20:43:31, Michael Stack wrote: > I did a rebase. So it picked up some other changes if you diff 2 from 3. On 2012-04-19 20:43:31, Michael Stack wrote: > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java, line 50 > < https://reviews.apache.org/r/4714/diff/2/?file=102179#file102179line50 > > > There are a bunch of import changes here. Are they all needed? It must be from other patch. This patch doesn't touch this file. On 2012-04-19 20:43:31, Michael Stack wrote: > security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java, line 187 > < https://reviews.apache.org/r/4714/diff/2/?file=102179#file102179line187 > > > Why can we get away w/ removing the try/catch? Because the caller handles it? That change in diff2 is already checked in as part of 5443 addendum. That's why it shows some change here. This patch doesn't change this file. On 2012-04-19 20:43:31, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 411 > < https://reviews.apache.org/r/4714/diff/2-3/?file=102195#file102195line411 > > > Does this belong in this patch? Is it part of another patch? From another patch due to rebasing. On 2012-04-19 20:43:31, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 2220 > < https://reviews.apache.org/r/4714/diff/2-3/?file=102195#file102195line2220 > > > Yeah, this stuff is from another patch? Why you adding it? From another patch due to rebasing. On 2012-04-19 20:43:31, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 2214 > < https://reviews.apache.org/r/4714/diff/2-3/?file=102203#file102203line2214 > > > Are these from Elliotts' patch? > > Maybe its reviewboard that is messing up? I'm only looking at diff between your v2 and v3 patch. From another patch due to rebasing. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/#review7046 ----------------------------------------------------------- On 2012-04-19 17:46:17, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-19 17:46:17) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50 src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8 src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995 src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10 Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. 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/4714/#review7051
      -----------------------------------------------------------

      Ship it!

      I'm good w/ this patch. Good stuff Jimmy

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java
      <https://reviews.apache.org/r/4714/#comment15645>

      Please file an issue to address this recursion (an AdminProtocol 'has' a HConnection but you get the AdminProtocol from an HConnection).

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
      <https://reviews.apache.org/r/4714/#comment15646>

      This is a noop

      • Michael

      On 2012-04-19 17:46:17, Jimmy Xiang wrote:

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

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

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

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

      (Updated 2012-04-19 17:46:17)

      Review request for hbase.

      Summary

      -------

      This is the admin part of HBase-5443. AdminProtocol part.

      This addresses bug HBASE-5621.

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

      Diffs

      -----

      src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79

      src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION

      src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION

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

      src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a

      src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9

      src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50

      src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb

      src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15

      src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443

      src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d

      src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8

      src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9

      src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a

      src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe

      src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865

      src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a

      src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830

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

      src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe

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

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988

      src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d

      src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995

      src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2

      src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c

      src/main/protobuf/Admin.proto 132c5dd

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

      src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b

      src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91

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

      src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de

      src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e

      src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27

      src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152

      src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd

      src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e

      src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c

      src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45

      src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10

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

      Testing

      -------

      All unit tests passed.

      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/4714/#review7051 ----------------------------------------------------------- Ship it! I'm good w/ this patch. Good stuff Jimmy src/main/java/org/apache/hadoop/hbase/client/HConnection.java < https://reviews.apache.org/r/4714/#comment15645 > Please file an issue to address this recursion (an AdminProtocol 'has' a HConnection but you get the AdminProtocol from an HConnection). src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java < https://reviews.apache.org/r/4714/#comment15646 > This is a noop Michael On 2012-04-19 17:46:17, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4714/ ----------------------------------------------------------- (Updated 2012-04-19 17:46:17) Review request for hbase. Summary ------- This is the admin part of HBase-5443. AdminProtocol part. This addresses bug HBASE-5621 . https://issues.apache.org/jira/browse/HBASE-5621 Diffs ----- src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 src/main/java/org/apache/hadoop/hbase/client/HTable.java 2c87d50 src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java cd4cccb src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 2fc4a15 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java 57c9443 src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java 52d179d src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 09601b8 src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java d0570b9 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 7239c5a src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java ecaf9fe src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 61a5988 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 759633d src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 7c59995 src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 66156c2 src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java 83a165c src/main/protobuf/Admin.proto 132c5dd src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 7ffd6bd src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 6b64f10 Diff: https://reviews.apache.org/r/4714/diff Testing ------- All unit tests passed. Thanks, Jimmy
      Hide
      stack added a comment -

      Want to put your patch up here Jimmy and run it by hadoopqa? Thanks.

      Show
      stack added a comment - Want to put your patch up here Jimmy and run it by hadoopqa? Thanks.
      Hide
      Jimmy Xiang added a comment -

      @Stack, thanks a lot for reviewing it.

      Show
      Jimmy Xiang added a comment - @Stack, thanks a lot for reviewing it.
      Hide
      Hadoop QA added a comment -

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

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

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

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

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

      -1 findbugs. The patch appears to introduce 9 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.regionserver.TestAtomicOperation
      org.apache.hadoop.hbase.constraint.TestConstraint
      org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol
      org.apache.hadoop.hbase.util.TestHBaseFsck
      org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithRemove

      Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1586//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1586//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1586//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/12523432/hbase-5621_v3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 9 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.regionserver.TestAtomicOperation org.apache.hadoop.hbase.constraint.TestConstraint org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol org.apache.hadoop.hbase.util.TestHBaseFsck org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithRemove Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1586//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1586//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1586//console This message is automatically generated.
      Hide
      Jimmy Xiang added a comment -

      Looking into the failed unit tests.

      Show
      Jimmy Xiang added a comment - Looking into the failed unit tests.
      Hide
      Jimmy Xiang added a comment -

      Minor changes to fix TestHBaseFsck failure and some security test failures.

      Show
      Jimmy Xiang added a comment - Minor changes to fix TestHBaseFsck failure and some security test failures.
      Hide
      Hadoop QA added a comment -

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

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

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

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

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

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

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

      -1 core tests. The patch failed these unit tests:
      org.apache.hadoop.hbase.client.TestAdmin

      Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1598//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1598//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1598//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/12523657/hbase_5621_v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 9 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.client.TestAdmin Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1598//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1598//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1598//console This message is automatically generated.
      Hide
      stack added a comment -

      Retrying

      Show
      stack added a comment - Retrying
      Hide
      Hadoop QA added a comment -

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

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

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

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

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

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

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

      -1 core tests. The patch failed these unit tests:

      Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1599//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1599//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1599//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/12523663/hbase_5621_v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 9 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1599//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1599//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1599//console This message is automatically generated.
      Hide
      stack added a comment -

      We seem to have run a good few less tests, ~880 vs 906 or so.... Does it pass all tests for you Jimmy? Thanks.

      Show
      stack added a comment - We seem to have run a good few less tests, ~880 vs 906 or so.... Does it pass all tests for you Jimmy? Thanks.
      Hide
      Ted Yu added a comment -

      For ServerManager.java:

      -    HRegionInterface hri = getServerConnection(server);
      +    AdminProtocol hri = getServerConnection(server);
      

      Can we use a better variable name to hold the AdminProtocol ? Consider how it is used below:

      +    return ProtobufUtil.openRegion(hri, region, versionOfOfflineNode);
      

      region is already HRegionInfo. hri used to denote HRegionInfo. Now it represents AdminProtocol.

      Show
      Ted Yu added a comment - For ServerManager.java: - HRegionInterface hri = getServerConnection(server); + AdminProtocol hri = getServerConnection(server); Can we use a better variable name to hold the AdminProtocol ? Consider how it is used below: + return ProtobufUtil.openRegion(hri, region, versionOfOfflineNode); region is already HRegionInfo. hri used to denote HRegionInfo. Now it represents AdminProtocol.
      Hide
      Jimmy Xiang added a comment -

      @Stack, let me check it again how many passed tests I got.

      @Ted, sure. Let me fix the naming issues.

      Show
      Jimmy Xiang added a comment - @Stack, let me check it again how many passed tests I got. @Ted, sure. Let me fix the naming issues.
      Hide
      Jimmy Xiang added a comment -

      I addressed the naming issue Ted pointed out.

      I think I got all regular tests passed: 537 small + 648 medium + 297 large = 1482

      With security, there are couple tests failed. I am looking into them. I don't think it's caused by this patch. Should we handle them in a different jira?

      Show
      Jimmy Xiang added a comment - I addressed the naming issue Ted pointed out. I think I got all regular tests passed: 537 small + 648 medium + 297 large = 1482 With security, there are couple tests failed. I am looking into them. I don't think it's caused by this patch. Should we handle them in a different jira?
      Hide
      Ted Yu added a comment -

      Can you list the tests that failed on your machine ?

      For security profile, TestProcessBasedCluster.testProcessBasedCluster fails consistently and is tracked by HBASE-5851.
      Other than that test, we should be careful.

      Show
      Ted Yu added a comment - Can you list the tests that failed on your machine ? For security profile, TestProcessBasedCluster.testProcessBasedCluster fails consistently and is tracked by HBASE-5851 . Other than that test, we should be careful.
      Hide
      Jimmy Xiang added a comment -

      Good to know there is already a jira to track TestProcessBasedCluster.testProcessBasedCluster failure.

      Besides that one, I also have this one failed sometimes (not always): TestServerCustomProtocol. It is flaky.

      Show
      Jimmy Xiang added a comment - Good to know there is already a jira to track TestProcessBasedCluster.testProcessBasedCluster failure. Besides that one, I also have this one failed sometimes (not always): TestServerCustomProtocol. It is flaky.
      Hide
      Hadoop QA added a comment -

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

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

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

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

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

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

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

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

      Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1609//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1609//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1609//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/12523812/hbase_5621_v5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestAssignmentManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1609//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1609//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1609//console This message is automatically generated.
      Hide
      Jimmy Xiang added a comment -

      TestAssignmentManager passed locally.

      For secure profile, TestHRegion also has some issue.

      Show
      Jimmy Xiang added a comment - TestAssignmentManager passed locally. For secure profile, TestHRegion also has some issue.
      Hide
      stack added a comment -

      TestHRegion has a problem the way its written. Will fix in 5833 commit that is coming up.

      Show
      stack added a comment - TestHRegion has a problem the way its written. Will fix in 5833 commit that is coming up.
      Hide
      stack added a comment -

      Applied to trunk. Thanks for the patch Jimmy.

      Show
      stack added a comment - Applied to trunk. Thanks for the patch Jimmy.
      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:
          1 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development