Review Board 1.7.22

HBASE-5445, First draft of Protobufs specification for HMasterInterface

Review Request #4283 - Created March 10, 2012 and updated

Gregory Chanan
First draft of the protobufs specification for HMasterInterface.
This is relatively close to a one-to-one mapping with the existing interface.  A pdf listing the existing-to-protobufs mapping is available on the JIRA:

Thanks to Devaraj who provided some initial work he had done on this.

Posted (March 13, 2012, 5:33 a.m.)
Looks good. Just a few comments below.
  1. Thanks for the review.  Will address in next patch.
src/main/proto/HMasterProtocol.proto (Diff revision 1)
Reference your nice PDF?

BTW, in your PDF, you have one method that is 'not used' so you don't include it.  Do we need to deprecate it in 0.94 so its safe to remove by the time 0.96 comes around?  If so, mind filing an issue or just me know and I can take care of it.

Ditto for loadBalancerSwitchIs(LoadBalancerSwitchIsRequest).  This method is just broke from its name and to how it works.  Can we deprecate it in 0.94?  Add a method that makes sense in 0.94, one that makes sense that you can redo in pb?

Otherwise, the pdf looks good.
  1. I got rid of "public void assign(final byte [] regionName, final boolean force)" which is already deprecated, so nothing to do for 0.94.
    Do you have any recommendations for:
    "public boolean balanceSwitch(final boolean b)" and "public boolean synchronousBalanceSwitch(final boolean b)"?
    which is what I combined into loadBalancerSwitchIs.  I thought "switch" was confusing because it is unclear if that is a verb (i.e. it switches whether the load balancer is running, so non-idempotent), vs a noun (it is a switch like an oven switch [on/off], so idempotent).  I think a noun is the correct way to think about it; I thought "SwitchIs" makes it clearer that its a noun.  On second though, I think I'd just call it "loadBalancerIs(final bool on,...)."  Should I add that method and deprecate the existing two methods above in 0.94?  I thought we were going to break everything in 0.96 anyway?
src/main/proto/HMasterProtocol.proto (Diff revision 1)
Did Benoit comment that protobuf is too long, just do pb (Ask Jimmy?  He'd remember)
src/main/proto/HMasterProtocol.proto (Diff revision 1)
Is this a good name for this class?  Drop the H?  Can we change it?
  1. I'll drop the H everywhere I can :).
src/main/proto/HMasterProtocol.proto (Diff revision 1)
These are in the master protocol but are they used elsewhere?  If so, should they be in here?  Maybe they are not and its just me confused.
  1. I don't think they are used elsewhere, will check.
src/main/proto/HMasterProtocol.proto (Diff revision 1)
Is this class deprecated (HServerInfo?)
  1. This is equivalent to HServerLoad.
src/main/proto/HMasterProtocol.proto (Diff revision 1)
Yeah, this method is broke as said above.  We should try do patch up work in 0.94 so you can come in all clean and shiny in 0.96.
src/main/proto/hbase.proto (Diff revision 1)
The package in first class is protobuf.generated.  Here its generated.protobuf.  Did you mean that?
  1. Good catch.
src/main/proto/hbase.proto (Diff revision 1)
What is one of these?  A bit of a comment?
src/main/proto/hbase.proto (Diff revision 1)
ditto.  Whats this map too?
src/main/proto/hbase.proto (Diff revision 1)
We used to 'guess' this from the bytes passed.  You are changing that?
src/main/proto/hbase.proto (Diff revision 1)
And we'd generate the encoded name from these articles?
src/main/proto/hbase.proto (Diff revision 1)
uint32 seems wide for number of stores
src/main/proto/hbase.proto (Diff revision 1)
src/main/proto/hbase.proto (Diff revision 1)
This is ServerName?  Why call it ServerSpecifier?  It should be a different name?
  1. I was just matching it up so it would be consistent with RegionSpecifier -- I'll figure out something better for this.
src/main/proto/hbase.proto (Diff revision 1)
Should ColumnMetadata and TableMetadata have common type?
  1. They could be.  Having them separate lets us evolve them independently, though.