hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB
Date Fri, 06 Apr 2012 21:03:23 GMT

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

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


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


Good stuff Jimmy.  A few comments below.  Sorry I ramble at times.  Read to the end before
reacting because I change my mind as I work through the patch.  You forgot to add RequestConverter?


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

    Should these be in here?  Can they be elsewhere, as defines in RegionClassProtocol?



src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<https://reviews.apache.org/r/4629/#comment14788>

    I don't know how you get away with these removes but I'm glad to see them go -- anything
that shrinks this catalog package is good by me



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

    RegionClientProtocol is a bad name, no?  Should it be just ClientProtocol and AdminProtocol?



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

    this should be getClient?  We're going to get a client for a table?  Maybe should be getTableClient?



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

    Why we need a converter?



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

    Whats the NULL_CONTROLLER about?



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

    So we don't close scanners anymore?



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

    Why do we need to pollute HConnection w/ higher level notions such as RegionClientProtocol
(or ClientProtocol?).... hmmm ClientProtocol would be ok but not RegionClientProtocol...



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

    I wonder if this needs to be public and in the Interface?  Could it be an internal thing?
 How uses this thing that comes out?  Maybe it needs to be here but sure seems like an internal
thing: i.e. we pass hostname and port of a particular regionserver but then internally as
we go about the cluster the client will go to new regionservers and set up connections....

    
    Looking around though, it seems that while most uses of this method should be shut down:
e.g. over in catalog package and replaced by calls that go via HTable, there are a few places
we need this still probably .. as in the master sendRegionClose, etc. calls.   So you can't
get rid  of it.
    
    But this looks like it should be called RegionServerProtocol
    or ClientProtocol and not RegionClientProtocol.  It connects to a RegionServer not a Region.....
    
    Or what you think Jimmy?    When I look at the RegionClient proto file, is it true to
say that in each message, we MUST pass a region since we are going against a particular region...
which would mean this is indeed a RegionClientProtocol (you just have to specify coordinates
in two steps; 1. pass servername setting up connection, then 2. for any call on the connection,
need to specify the region.....)
    
    
    



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

    Move these defines into this class rather than up in HConstants?



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

    Whats happening here?  We want to drop the methods that took hostname and port only and
move to those that take servername?



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

    Is this good?  Should we have servername?  What if server restarts in between?  Would
we want to notice that we don't have a connection to new instantiation and go create one?



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

    Does this need to be synchronized at all?



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

    I suppose HBaseClient down in rpc knows it because it has to deserialize the pb?



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

    scannerid is internal or something now?



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

    Not your fault but server is a bad variable name for what this is, huh..



src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
<https://reviews.apache.org/r/4629/#comment14803>

    Do you have to?  This stuff is nasty enough w/o lettting it out of this package.



src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java
<https://reviews.apache.org/r/4629/#comment14804>

    Fix



src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java
<https://reviews.apache.org/r/4629/#comment14805>

    What is this for?



src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
<https://reviews.apache.org/r/4629/#comment14806>

    Is ServiceException a pb exception?



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

    Why not throw ServiceException?



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

    Ugh.  KeyValue needs to be an Interface that both pb and our current KV implements.
    
    This will be a performance killer?  Should Result let out the raw pb KV?



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

    Good one.  I should use this in stuff I committed recently



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

    Its as though we should give clients a way to pass in the raw pbs rather than have them
go via our versions of Get, Delete, etc.



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

    white space



src/main/java/org/apache/hadoop/hbase/protobuf/RegionAdminProtocol.java
<https://reviews.apache.org/r/4629/#comment14812>

    Is this just regionadmin protocol?  Or is it general admin?



src/main/java/org/apache/hadoop/hbase/protobuf/RegionClientProtocol.java
<https://reviews.apache.org/r/4629/#comment14813>

    This is probably a good idea.


- Michael


On 2012-04-03 23:32:10, Jimmy Xiang wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/4629/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-04-03 23:32:10)
bq.  
bq.  
bq.  Review request for hbase.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  This is the client protocol part of region interface. The admin protocol part will be
done in a different jira.
bq.  
bq.  The HRegionInterface is still there since the admin part is not done yet.  The other
reason is that in case some people still wants the old interface
bq.  
bq.  Filters, comparators and coprocessor parameters are still Writable.  They will be addressed
in different jiras.
bq.  
bq.  The existing client interface is not changed so that we don't break existing clients.
bq.  
bq.  
bq.  This addresses bug HBASE-5620.
bq.      https://issues.apache.org/jira/browse/HBASE-5620
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba 
bq.    src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java 0129ee9 
bq.    src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 97293aa 
bq.    src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 
bq.    src/main/java/org/apache/hadoop/hbase/client/HConnection.java 5d43086 
bq.    src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java aa30969 
bq.    src/main/java/org/apache/hadoop/hbase/client/HTable.java 3db0295 
bq.    src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java 9903df3 
bq.    src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java ddcf9ad 
bq.    src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1acbdab 
bq.    src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 
bq.    src/main/java/org/apache/hadoop/hbase/io/TimeRange.java d135393 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 05ae717 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 1316457 
bq.    src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 19ae18c

bq.    src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/RegionAdminProtocol.java PRE-CREATION

bq.    src/main/java/org/apache/hadoop/hbase/protobuf/RegionClientProtocol.java PRE-CREATION

bq.    src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java PRE-CREATION

bq.    src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java 4026da0 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java b36a9c0

bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 4f80999 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 703e73d

bq.    src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b520f3f 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java PRE-CREATION 
bq.    src/main/protobuf/RegionClient.proto 358382b 
bq.    src/main/protobuf/hbase.proto da78788 
bq.    src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java c7284dc 
bq.    src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 0042468

bq.    src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java e34d8bc

bq.    src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 
bq.    src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java d2b3060 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 227c5f2 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java ceca6f5 
bq.    src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java cac2989 
bq.    src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java a1bf73b

bq.  
bq.  Diff: https://reviews.apache.org/r/4629/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  "mvn -PrunAllTests clean test" is green, except some flaky tests which need to run again.
bq.  
bq.  Also tested it on a real cluster with ycsb and bigtop.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Jimmy
bq.  
bq.


                
> Convert the client protocol of HRegionInterface to PB
> -----------------------------------------------------
>
>                 Key: HBASE-5620
>                 URL: https://issues.apache.org/jira/browse/HBASE-5620
>             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

        

Mime
View raw message