Mailing-List: contact issues-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Date: Sun, 15 Apr 2012 16:37:17 +0000 (UTC)
From: "jiraposter@reviews.apache.org (Commented) (JIRA)" <jira@apache.org>
To: issues@hbase.apache.org
Message-ID: 
 <974912649.26800.1334507837814.JavaMail.tomcat@hel.zones.apache.org>
In-Reply-To: 
 <1405139827.3403.1332439102394.JavaMail.tomcat@hel.zones.apache.org>
Subject: [jira] [Commented] (HBASE-5621) Convert admin protocol of
 HRegionInterface to PB
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/HBASE-5621?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254364#comment-13254364 ] 

jiraposter@reviews.apache.org commented on HBASE-5621:
------------------------------------------------------


bq.  On 2012-04-13 23:59:40, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 2814
bq.  > <https://reviews.apache.org/r/4714/diff/1/?file=101799#file101799line2814>
bq.  >
bq.  >     Where has allt his code gone?
bq.  
bq.  Jimmy Xiang wrote:
bq.      Some are moved to RegionServer as I did for 5620
bq.  
bq.  Michael Stack wrote:
bq.      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.


bq.  On 2012-04-13 23:59:40, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ClientProtocol.java, line 29
bq.  > <https://reviews.apache.org/r/4714/diff/1/?file=101781#file101781line29>
bq.  >
bq.  >     Ditto
bq.  
bq.  Jimmy Xiang wrote:
bq.      Will move to client.
bq.  
bq.  Michael Stack wrote:
bq.      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.


bq.  On 2012-04-13 23:59:40, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/AdminProtocol.java, line 28
bq.  > <https://reviews.apache.org/r/4714/diff/1/?file=101780#file101780line28>
bq.  >
bq.  >     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)
bq.  >     
bq.  >     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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      Let me move it to client.
bq.  
bq.  Michael Stack wrote:
bq.      And it should not be public I'd say.

Yes, it is private.


bq.  On 2012-04-13 23:59:40, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 632
bq.  > <https://reviews.apache.org/r/4714/diff/1/?file=101782#file101782line632>
bq.  >
bq.  >     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.
bq.  >     
bq.  >     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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.
bq.  
bq.  Michael Stack wrote:
bq.      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.


bq.  On 2012-04-13 23:59:40, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 32
bq.  > <https://reviews.apache.org/r/4714/diff/1/?file=101784#file101784line32>
bq.  >
bq.  >     Did you say you were going to remove these from here?
bq.  
bq.  Jimmy Xiang wrote:
bq.      I tried but it is hard.  Let me think about it again.
bq.  
bq.  Michael Stack wrote:
bq.      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:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/4714/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-04-13 23:03:35)
bq.  
bq.  
bq.  Review request for hbase.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  This is the admin part of HBase-5443.  AdminProtocol part.
bq.  
bq.  
bq.  This addresses bug HBASE-5621.
bq.      https://issues.apache.org/jira/browse/HBASE-5621
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/AdminProtocol.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/ClientProtocol.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 408db79 
bq.    src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72 
bq.    src/main/java/org/apache/hadoop/hbase/client/HConnection.java 23f8e5a 
bq.    src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 820e2a9 
bq.    src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java fe80fcf 
bq.    src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java 2a9d86e 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 3a3a79f 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java b7afa58 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java dd0a1bc 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 9f159f2 
bq.    src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ab33ac7 
bq.    src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 70901fe 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 422e865 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 3d6a23a 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java b056830 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a912cc3 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java e78e56d 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 0d22c0e 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 9487a1c 
bq.    src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 04fe8b6 
bq.    src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 61f8128 
bq.    src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java ee4f556 
bq.    src/main/protobuf/Admin.proto 132c5dd 
bq.    src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java d6ae0e2 
bq.    src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 3cfc02b 
bq.    src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 8af0f91 
bq.    src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java aa7f51b 
bq.    src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java 7dd60de 
bq.    src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java 24f878e 
bq.    src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java 301ee27 
bq.    src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java a59e152 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b84a115 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java cedf31e 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 3122b15 
bq.    src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java c0ac12c 
bq.    src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java d0cad45 
bq.    src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a06942d 
bq.  
bq.  Diff: https://reviews.apache.org/r/4714/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  All unit tests passed.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Jimmy
bq.  
bq.


> Convert admin protocol of HRegionInterface to PB
> ------------------------------------------------
>
>                 Key: HBASE-5621
>                 URL: https://issues.apache.org/jira/browse/HBASE-5621
>             Project: HBase
>          Issue Type: Sub-task
>          Components: ipc, master, migration, regionserver
>            Reporter: Jimmy Xiang
>            Assignee: Jimmy Xiang
>             Fix For: 0.96.0
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira