hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5935) Add Region-level PB-based calls to HMasterInterface
Date Tue, 08 May 2012 22:47:50 GMT

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

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



bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > Looks good.  Some small potatoes below.  On this, "I've put the new generated calls
in HMasterInterface rather than a new class, because it is easier to have one class for now,
until I convert everything.  This will only be temporary, but let me know how you want to
handle.  Bump the version number?" ... when will HMasterInterface go away?  What else all
is to be done?  Sure on bump the version number.  Pretty soon they'll be ignored so no harm
using up a few.  Good stuff G.

Cool, I'll bump the version number for now.

I believe I've answered all your questions below.  Let me know if I missed anything.

Thanks for the review.


bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1213
bq.  > <https://reviews.apache.org/r/5060/diff/2/?file=107651#file107651line1213>
bq.  >
bq.  >     FYI, spaces between if and open paren... and usually around operators.  Just
FYI.

Thanks, will do.


bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 1394
bq.  > <https://reviews.apache.org/r/5060/diff/2/?file=107649#file107649line1394>
bq.  >
bq.  >     Is it right catching this here?  You seem to handle it at a higher level up
in the move invocation?   Won't this clause catch all the possible SEs?
bq.  >     
bq.  >     Or I'm suffering myopia  and there is more going on than just this call invocation?

Good point, I can catch at a higher level.


bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1219
bq.  > <https://reviews.apache.org/r/5060/diff/2/?file=107651#file107651line1219>
bq.  >
bq.  >     This needs paren (though its in the original)

Will do.


bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > src/main/protobuf/Master.proto, line 71
bq.  > <https://reviews.apache.org/r/5060/diff/2/?file=107654#file107654line71>
bq.  >
bq.  >     Great.  Is this all that is in the master interface?

No, the rest of the functions in HMasterInterface need to be converted.  This is the region-level
stuff, I still have column-level, table-level and miscellaneous stuff to do.  I've filed a
JIRA for the column-level stuff; I'll file JIRAs for the others once I figure out how I'm
going to break down the work.


bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1253
bq.  > <https://reviews.apache.org/r/5060/diff/2/?file=107651#file107651line1253>
bq.  >
bq.  >     All IOEs now need to get converted to a SE?  I added an HBaseException recently.
 Its a checked exception.  Should we have an HBaseServiceExceptoin?  SE is pb?

ServiceException is coming from pb, that's right.  So when we either get rid of HMasterInterface
or repurpose it, the interface will define that we throw ServiceExceptions.


bq.  On 2012-05-08 04:55:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 285
bq.  > <https://reviews.apache.org/r/5060/diff/2/?file=107650#file107650line285>
bq.  >
bq.  >     It does seem a little messy having to hoist all these pb classes up in here
AND into HMasterInterface.
bq.  >     
bq.  >     HMasterInterface is going to go away?

I'm not sure what you mean by "having to hoist all these pb classes up in here AND into HMasterInterface."
-- this is HMasterInterface?

Also, I don't think PB will be nice and generated usable javadoc for you, so I'm not sure
how to provide javadoc comments without redeclaring the functions?  I admit to being completely
ignorant about this area.

Once everything in HMasterInterface is converted to use PB, we can either declare a new class
for the representation (similar to RegionServerStatusProtocol) or just re-purpose HMasterInterface
for that.  What is your preference?


- Gregory


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


On 2012-05-08 01:34:20, Gregory Chanan wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/5060/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-05-08 01:34:20)
bq.  
bq.  
bq.  Review request for hbase, Michael Stack and Jimmy Xiang.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Converted all the region-level calls in HMasterInterface to use Protobufs (move, assign,
unassign).
bq.  
bq.  I've put the new generated calls in HMasterInterface rather than a new class, because
it is easier to have one class for now, until I convert everything.  This will only be temporary,
but let me know how you want to handle.  Bump the version number?
bq.  
bq.  
bq.  This addresses bug HBASE-5935.
bq.      https://issues.apache.org/jira/browse/HBASE-5935
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java ce81547 
bq.    src/main/java/org/apache/hadoop/hbase/master/HMaster.java ccc7119 
bq.    src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 5d4be3f 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java a5e03e1 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java PRE-CREATION

bq.    src/main/protobuf/Master.proto PRE-CREATION 
bq.    src/main/protobuf/RegionServerStatus.proto 9d7728f 
bq.    src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java e5de603 
bq.    src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java 83297cc 
bq.  
bq.  Diff: https://reviews.apache.org/r/5060/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Gregory
bq.  
bq.


                
> Add Region-level PB-based calls to HMasterInterface
> ---------------------------------------------------
>
>                 Key: HBASE-5935
>                 URL: https://issues.apache.org/jira/browse/HBASE-5935
>             Project: HBase
>          Issue Type: Task
>          Components: ipc, master, migration
>            Reporter: Gregory Chanan
>            Assignee: Gregory Chanan
>             Fix For: 0.96.0
>
>
> This should be a subtask of HBASE-5445, but since that is a subtask, I can't also make
this a subtask (apparently).
> This is for converting the region-level calls, i.e.:
> moveRegion
> assignRegion
> unassignRegion

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