Activity

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

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?

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?

jiraposter@reviews.apache.org
added a comment - 14/Apr/12 01:01
-----------------------------------------------------------
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

> 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.

> 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?

jiraposter@reviews.apache.org
added a comment - 14/Apr/12 01:31
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

> 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.

jiraposter@reviews.apache.org
added a comment - 14/Apr/12 05:11
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

> 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.

jiraposter@reviews.apache.org
added a comment - 15/Apr/12 17:37
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

-----------------------------------------------------------
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.

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

Hadoop QA
added a comment - 22/Apr/12 05:41 -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.

Ted Yu
added a comment - 22/Apr/12 16:15 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.

Jimmy Xiang
added a comment - 23/Apr/12 17:27 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?

Ted Yu
added a comment - 23/Apr/12 17:49 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.

Jimmy Xiang
added a comment - 23/Apr/12 18:08 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.