HBASE-3256: Coprocessors: Coprocessor host and observer for HMaster

Review Request #1321 - Created Dec. 20, 2010 and updated

Gary Helmling
apurtell, jgray, stack
This patch adds a new MasterObserver interface with pre/post hooks provided for operations defined in org.apache.hadoop.hbase.ipc.HMasterInterface.

In order to accommodate the new MasterObserver interface, I've also refactored out common coprocessor base code, with subclasses providing for region-specific and master-specific behavior.

The new code structure is (excuse my poor ascii art):

CoprocessorEnvironment - base interface for common facilities provided to CP implementations
    |- RegionCoprocessorEnvironment - adds access to current HRegion and RegionServerServices (for RegionObservers)
    |- MasterCoprocessorEnvironment - adds access to MasterServerServices (for MasterObservers)

CoprocessorHost - abstract base providing core CP loading and invocation code and the base CoprocessorEnvironment implementation
    |- RegionCoprocessorHost - provides hooks for invoking RegionObserver pre/post methods and RegionCoprocessorEnvironment implementation
    |- MasterCoprocessorHost - provides hooks for invoking MasterObserver pre/post methods and MasterCoprocessorEnvironment implementation

Also added:
 - org.apache.hadoop.hbase.coprocessor.BaseMasterObserver - stubs out full MasterObserver interface with empty methods for convenience
 - org.apache.hadoop.hbase.coprocessor.TestMasterObserver - tests that MasterObserver pre/post methods are called during master operations.

In particular, please let me know if the MasterObserver method inputs and outputs are sufficient for whatever you anticipate doing with it.  It should meet our needs for security checks, but more input would be helpful.
Added a new test (org.apache.hadoop.hbase.coprocessor.TestMasterObserver) to cover pre/post hook invocation.

All existing coprocessor tests still pass.
  2. If we were to allow override of assignments, it would have to happen here. If the cp calls bypass() then return immediately.
  3. Likewise if we were to allow overriding assignment, we need a symmetrical operation here.
  1. great work!  just a few small comments but otherwise +1
  2. does DEFAULT really mean REGION/REGIONSERVER?  or is it both?
    not a big deal if it's just variable names but since it's a config param, we should nail it now before it gets out in a release.
  3. this code might have been in other earlier patches but could there be false positives with this?  it'd be silly to load FancyCoprocessor and then MyFancyCoprocessor but i guess this is to cover the package?  maybe parse out the class name?
  4. doesn't preBalance() return a void?  it's preBalanceSwitch that returns boolean
  5. and here we should get the boolean return value (and base class should return the input value)
  6. would we ever want to override default assign behavior?  it's feasible... might want to be future proof w/ the api?
  2. It actually means "region".  That conf key is only used for the system coprocessors loaded on regions.
    I'll change the name (and config property).
  3. Yes, I'm not sure what the original intent was here, obtaining the CP without the full package name?
    Maybe getClass().getSimpleName().equals() would be better?
  4. This is the result of env.shouldBypass(), in order to allow a MasterObserver to bypass the normal balance() processing.
  5. Right, that's the only way to modify the input.  Will change.
  6. I can add in env.shouldBypass() handling here to allow overriding.  Combined with access to ServerManager through MasterServices, this should allow custom assignment policies.
  7. Yes, will add in env.shouldBypass() handling here too.
Review request changed

Change Summary:

Changes in response to review comments:
 - DEFAULT -> REGION in var name and property
 - CoprocessorHost.findCoprocessor(): use getClass().getSimpleName().equals() instead of getClass().getName().endsWith() for fallback
 - add bypass handling for MasterCoprocessorHost.preAssign() and preUnassign()
 - use return value from MasterObserver.preBalanceSwitch() to allow modifying input


Revision 2 (+3367 -1589)

Show changes

  1. looks good.  clean and nicely commented, well done.