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

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



bq.  On 2012-04-12 04:50:15, Michael Stack wrote:
bq.  > What you want to do Jimmy?  On next iteration, if all tests pass, should 
we check it in?  I think its missing unit tests for the new protobuf utility.  
We should add those.  I'd be fine adding them in a separate patch if that would 
suit you.  This thing is already massive.  We could commit to a branch too but 
that probably doesn't get us far; you have your own setup where you are to 
which you can commit, right?

Could you please check it in if all tests pass?  It can save me some 
rebasing/merging efforts. :)

I can add unit tests for the new protobuf utility in a separate patch.  I do 
have my own setup to commit my changes.  I am also working on HBASE-5621.
I can resolve other issues in HBASE-5621, or separate patches.


bq.  On 2012-04-12 04:50:15, Michael Stack wrote:
bq.  > src/main/protobuf/Client.proto, line 56
bq.  > <https://reviews.apache.org/r/4629/diff/2/?file=100900#file100900line56>
bq.  >
bq.  >     Call this keyValueBytes then so method  names give more of a clue as 
to what you'll be getting; save on the surprises?

Sure, will fix.


bq.  On 2012-04-12 04:50:15, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, 
line 143
bq.  > <https://reviews.apache.org/r/4629/diff/2/?file=100898#file100898line143>
bq.  >
bq.  >     Should this class implement Closeable, Stoppable and Abortable?

RegionServerServices defined Stoppable, Abortable. Now RegionServer implements 
RegionServerServices.
Not sure about Closeable.


bq.  On 2012-04-12 04:50:15, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, 
line 98
bq.  > <https://reviews.apache.org/r/4629/diff/2/?file=100898#file100898line98>
bq.  >
bq.  >     Shouldn't this implement Server and RegionServerServices?
bq.  >     
bq.  >     I would love to take the time to break this class up into smaller 
pieces but thats not for this JIRA.  Can do that after this goes in.

RegionServer now implements RegionServerServices which extends Server.

It will be great to break this class up.


bq.  On 2012-04-12 04:50:15, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, 
line 94
bq.  > <https://reviews.apache.org/r/4629/diff/2/?file=100898#file100898line94>
bq.  >
bq.  >     Fix comment.
bq.  >     
bq.  >     Should you explain your intent for this class too?  That 
HRegionServer will wither away?  I think it woud be useful since it was not 
plain to me at first but after you explained it, I was on your side.

Sure, added comments.  Thanks.


- Jimmy


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


On 2012-04-07 21:30:32, 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-07 21:30:32)
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/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 
0b783ce 
bq.    src/main/java/org/apache/hadoop/hbase/client/HTable.java aa7652f 
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/WritableRpcEngine.java 1316457 
bq.    
src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 
19ae18c 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 
PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 
PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de 
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/AdminProtos.java 
PRE-CREATION 
bq.    
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.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/RegionAdminProtos.java 
2169310 
bq.    
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
 b36a9c0 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 
8a61f7d 
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/Admin.proto PRE-CREATION 
bq.    src/main/protobuf/Client.proto PRE-CREATION 
bq.    src/main/protobuf/RegionAdmin.proto c64d68b 
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 
41616c8 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 
6ed4ba2 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 
b4dcb83 
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

        

Reply via email to