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 Sat, 07 Apr 2012 00:05:34 GMT

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

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



bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 89
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line89>
bq.  >
bq.  >     What does this do?  Needs class comment

Will add comment.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 97
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line97>
bq.  >
bq.  >     Spelling

Good catch, will fix.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 98
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line98>
bq.  >
bq.  >     We only do the latter if the flag is set, right?

Right.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 107
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line107>
bq.  >
bq.  >     Is this a get request builder?  Name method getRequestBuilder?

It uses a request builder to build a get request.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 146
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line146>
bq.  >
bq.  >     Do you think that we should eventually just deprecate the current Get, Delete,
and Put, etc. and move to the pb ones altogether?  They are only in the way now?

I think so.  We don't want to break the existing applications.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 167
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line167>
bq.  >
bq.  >     buildMutateRequest?

Sounds good to me.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, line 646
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98835#file98835line646>
bq.  >
bq.  >     Should this stuff be public?  Its internal stuff?  Should it be over under the
client package?  Then you could keep it encapsulated at least w/i the package?  Is it used
outside of the client package?

Let me check. I will try not to expose it.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, line 56
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98836#file98836line56>
bq.  >
bq.  >     Ditto.  Should this be over in the client package?  Is that the only place its
used?  Could we make it non-public?

Will do if it works.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, line 101
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98836#file98836line101>
bq.  >
bq.  >     buildActionResult?

Sure.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 185
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line185>
bq.  >
bq.  >     We don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 193
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line193>
bq.  >
bq.  >     Why we don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 218
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line218>
bq.  >
bq.  >     We don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 229
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line229>
bq.  >
bq.  >     We don't need any more?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 271
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line271>
bq.  >
bq.  >     We don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 459
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line459>
bq.  >
bq.  >     For sure these are no longer needed?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1119
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line1119>
bq.  >
bq.  >     Not needed anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1155
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line1155>
bq.  >
bq.  >     Not used anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1181
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line1181>
bq.  >
bq.  >     Why this no more?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1199
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line1199>
bq.  >
bq.  >     Why this no more?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 2518
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line2518>
bq.  >
bq.  >     No more of this?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 3094
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line3094>
bq.  >
bq.  >     Ain't this needed?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 3195
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98839#file98839line3195>
bq.  >
bq.  >     Some pretty radical removals in here.  For sure we don't need the stuff being
removed (Otherwise, I'm glad to see it go)

Moved to RegionServer, a new base class.  I was think to remove class HRegionServer later
on when HRegionInterface is not needed.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 98
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98842#file98842line98>
bq.  >
bq.  >     oh... I think I like what I'm seeing.  Could this be an Interface at all?

It is the new pb version of HRegionServer.  So it is a class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/protobuf/RegionClient.proto, line 150
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98843#file98843line150>
bq.  >
bq.  >     We don't need these?

I added DeleteType to handle such usecases.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 1162
bq.  > <https://reviews.apache.org/r/4629/diff/1/?file=98842#file98842line1162>
bq.  >
bq.  >     What is the advantage of doing this?

So I don't mess HRegionInterface with the new pb. Later on, it will be easier to remove HRegionInterface.


- Jimmy


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


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