Review Board 1.7.22


HBASE-5444: Add PB-based calls to HMasterRegionInterface

Review Request #4463 - Created March 23, 2012 and updated

Gregory Chanan
0.96
HBASE-5444
Reviewers
hbase
stack
hbase-git
Adds PB-based calls replacing HMasterRegionInterface.

There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes).  That will be cleaned up in HBASE-5445.
Ran jenkins job, all unit tests passed.
Review request changed
Updated (May 2, 2012, 11:19 p.m.)
Make changes based on Ted's and Stack's comments.

- Fixed spacing
- Moved RegionServerStatusProtocol from master to ipc (not o.a.h.h -- hopefully that's okay)
- Created a ServerLoad object that encapsulated the PB ServerLoad object.
Posted (May 2, 2012, 11:43 p.m.)
I'm +1 on this patch.  I think this much cleaner than previous versions.  I wanted more, of course, where users of the protocol would somehow be untouched or polluted by pbs but I realize that is asking for to much.  Good stuff Gregory.
We need this import?  Its for cp.  Thats ok I'd say.... One day we can hide that too..
  1. I think we can get rid of this once I pb-ify the HMasterInterface.  I'll put it on my list to check out.  There's probably a few such cases.
We can move the ipc protocol stuff to top level later... I was thinking that these classes shared by master and regionservers could be at o.a.h.h... but can do that later if it makes sense.  Lets get this pb stuff in first.

  1. Sounds good.
We need this?
  1. I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands.  Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName.  We already have a bunch of these parse* functions already, e.g.
    
    public static ServerName parseVersionedServerName(final byte [] versionedBytes)
  2. Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes.  Could use this.
  3. I don't think this applies.  From reading ServerName.parseFrom it looks like it requires the PBMagicPrefix, which this case doesn't have nor need.  Am I missing something?
  4. If no pb magic, it then goes on to try and parse the bytes otherwise.  See other side of the pb check starting at line #332.
Yeah, I suppose you can't hide these from the class that is implementing the protocol...