HBASE-2001 RegionObserver

Review Request #96 - Created May 26, 2010 and discarded

Andrew Purtell
This patch is the parts of the HBASE-2001 patch which implements support for the RegionObserver interface. This enables extension of the regionserver through stacking dynamically loaded classes i.e. from jars on HDFS onto upcalls from HRegion. I made some improvements over the other patch and added a test case. There are other parts of 2001 which need some thought and some work and would not be useful without client side support. This is the part which could be immediately useful. 

Submitted for feedback. 

Incorporates a user suggestion and Stack +1 about hooking compaction.
All the new unit tests plus TestHRegion pass locally.
Review request changed
  1. this seems like a reasonable framework, but I'd rather see this stay around as a branch until there is at least one or two actual things using it for a real purpose. Otherwise I think we'll end up shipping an API that we later realize doesn't work for real apps. What do you think?
    1. This seems like a good idea when dev'ing an Interface.  After the 3rd implemenation you'll have some confidence in your Interface.
    2. Ok, I'll take all the comments below and incorporate the feedback in a new version and commit it on a feature branch that will track trunk. I can use git to manage the merging and just push snapshots into SVN. Will set up a project on our Hudson to crunch tests for it.
  2. before or after reporting?
    1. Should be just before. Will double check.
  3. this map-like interface is somewhat confusing - what's the purpose of it?
    1. This is an environment space, like unix process env vars, shared among all threads of the coprocessor (which get a reference to the environment). Useful for the mapreduce stuff not included in this patch. For example, rather than sum or average using intermediates, update AtomicLongs instead.
  4. woah, can we add something to the build to build this jar as a test resource, or something?
  5. you could use mockito verification to do this, probably would be simpler
    1. Thanks this would be a good opportunity to learn mockito.
  1. This looks really great Andrew.
  2. What about unloading?  You remember that conversation up on irc of how loading is one thing but unloading w/o breakage is hard prob.
    1. I'm not trying to tackle unloading yet.
      However, classes are strongly bound to their classloader. We do instantiate a classloader each time to load a coprocessor and we don't hold a reference to the classloader. It is my understanding that when there are no more references to the classes (no live objects), they and the classloader will be garbage collected, though the JVM spec does not guarantee this the Sun JVM will do this. Creating a new classloader and asking for the class again, presumably from an updated jar, should load the new class -- a unit test can verify. 
      To help insure old classes don't leak via live objects hanging around, we could consider a cooperative lifecycle management scheme like that used by OSGi: http://en.wikipedia.org/wiki/OSGi. 
  3. I love the one where fellas would be allowed intercept compactions adding their own compacting algorithm -- but I suppose that out of scope here, or coprocessors v2? (nm... I see it later in this patch)
  4. Great doc.  Some could probably go out into package doc...
  5. Make this javadoc?
  6. Which table is this?
    1. Any table. The idea is the coprocessor can create any private tables it needs to implement its functionality.
  7. Needs to be Writable?
  8. This is a mixin you'd use if you want to be notified about compactions, etc.?
    1. This is for translating values found in a flush file into new values in the new storefile being built by the compaction, or for dropping values. 
  9. What would this be used for?  For CP to call out elsewhere on a table?
    1. The idea is the coprocessor can create any private tables it needs to implement its functionality. But we want to mediate that, add access control, clean up references when/if the cp is terminated (and perhaps unloaded).
  10. Write this as LOG.warn("Failed close of table=" + table, e)?
  11. How does this get triggered inside the HRS?  I suppose I'll learn later down in this patch.
  12. Its always on then?
    1. Yes, otherwise I have to wrap all calls to the cp host in HRegion with "if (coprocessorHost != null) then", including the inner loops of the major and minor compactors. 
  13. Who would want to get at this?
    1. Tests. So probably this does not have to be public.
    1. This is awesome stuff Andrew!  Great work!
      I agree with Todd, this would probably benefit from baking in a branch somewhere for a while.  Start working up some solid examples to iron out the API.
      There are also a number of master changes as well as compact/flush/split changes that are going into trunk soon that will impact many of the hooks.
      Also, I recall you previously built some type of lightweight MR with coprocessors?  How does that work and how would it fit in with this?
  2. What is purpose of this?
    1. Coprocessors may want to change their behavior based on different minor versions. 
  3. Split decisions will not be made post-compaction as they are now after HBASE-2375 goes in.  That decision will actually be made at flush time, most likely post-flush though we'll know at the start whether it will end up needing to split.
    1. Then this signal should shift to the flush upcall.
  4. So a coprocessor implementation would potentially implement Coprocessor and RegionObserver?  Notifications of higher level events happen through Coprocessor, this is for lower level hooks?  Maybe a bit more detail in class comment to describe difference between the two interfaces.
    1. RegionObserver used to have a cleaner distinction from Coprocessor. Initially Coprocessor hooked internal region housekeeping; meanwhile RegionObserver wrapped HRegionInterface. 
  5. This makes sense now reading the rest of the code.  But it seems that the Coprocessor is in fact the "observer" that just gets notified of actions while this observer is actually the "processor" that can manipulate stuff?
    1. An interesting idea to consider renaming the interfaces for clarity.
  6. And descending timestamp
  7. This is great javadoc
  8. Gets are called after the Get is performed, Puts and Deletes are called before, correct?
    Would there be a use case for pre-Get hook?  Just wondering.
    1. I wanted for the interface to be able to munge the result of the Get and only was considering one "onGet". But for sure we could do "onGet" and "onGetResult". 
  9. Javadoc says it's called after the split happens but before report to master.  Seems that this happens once we create the new HRegions but before we actually do the swap.  What exactly would/could a coprocessor be doing in this window?
    One thing to be aware of is the master changes coming are going to make a split run entirely on the RS including the edits to META, closing of the parent, and opening of the children.  Where in that process would this hook make sense?
    1. This is so a CP on a parent can know in advance that daughters from a split are about to come on line, and their associated CPs are about to be initialized and become operational. I don't know how useful this would be but it seemed a reasonable part of the region lifecycle to intercept.