Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.92.0
    • Component/s: Coprocessors
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Support user code that runs run next to each region in table. As regions split and move, coprocessor code should automatically move also.

      Use classloader which looks on HDFS.

      Associate a list of classes to load with each table. Put this in HRI so it inherits from table but can be changed on a per region basis (so then those region specific changes can inherited by daughters).

      Not completely arbitrary code, should require implementation of an interface with callbacks for:

      • Open
      • Close
      • Split
      • Compact
      • (Multi)get and scanner next()
      • (Multi)put
      • (Multi)delete

      Add method to HTableInterface for invoking coprocessor methods and retrieving results.

      Add methods in o.a.h.h.regionserver or subpackage which implement convenience functions for coprocessor methods and consistent/controlled access to internals: store access, threading, persistent and ephemeral state, scratch storage, etc.

      GitHub: https://github.com/trendmicro/hbase/tree/coprocessor

      Please see the latest attached package-info.html for updated description.

      1. HBase-2001-final.patch
        161 kB
        Mingjie Lai
      2. packge-info.html
        13 kB
        Mingjie Lai
      3. packge-info.html
        13 kB
        Mingjie Lai
      4. packge-info.html
        12 kB
        Mingjie Lai
      5. HBASE-2001-RegionObserver-2.patch
        85 kB
        Mingjie Lai
      6. HBASE-2001-RegionObserver.patch
        75 kB
        Andrew Purtell
      7. HBASE-2001.patch.gz
        23 kB
        Andrew Purtell
      8. asm-transformations.pdf
        76 kB
        Andrew Purtell

        Issue Links

          Activity

          Hide
          Andrew Purtell added a comment -

          One application of this is like in process map reduce on the HBase cluster. Coprocessors accept list of writables in and produce a list of writables out, like a mapper. Client library doing collation and aggregation of per region invocations would be functionally like a reducer.

          Show
          Andrew Purtell added a comment - One application of this is like in process map reduce on the HBase cluster. Coprocessors accept list of writables in and produce a list of writables out, like a mapper. Client library doing collation and aggregation of per region invocations would be functionally like a reducer.
          Hide
          stack added a comment -

          HBASE-1936 has a patch that will do classloading off hdfs.

          Show
          stack added a comment - HBASE-1936 has a patch that will do classloading off hdfs.
          Hide
          Andrew Purtell added a comment -

          Consider ASM (http://asm.ow2.org/), a high performance bytecode rewrite framework with BSD style license. Just another jar to drop into lib/.

          • Take additional verification steps beyond basic bytecode verification to reject object and/or method use beyond those on a white list if security policy demands it
          • Insert code to deal with regionserver internal details that client code should not be concerned about
          • Weave in CPU and/or memory accounting and/or forced termination should bounds on same be exceeded
          • Rewrite calls to an interface into static invocations of the implementer
          • Weave in auditing
            etc.
          Show
          Andrew Purtell added a comment - Consider ASM ( http://asm.ow2.org/ ), a high performance bytecode rewrite framework with BSD style license. Just another jar to drop into lib/. Take additional verification steps beyond basic bytecode verification to reject object and/or method use beyond those on a white list if security policy demands it Insert code to deal with regionserver internal details that client code should not be concerned about Weave in CPU and/or memory accounting and/or forced termination should bounds on same be exceeded Rewrite calls to an interface into static invocations of the implementer Weave in auditing etc.
          Hide
          Jeff Hammerbacher added a comment -

          Chiming in to note that Doug is using ASM for some stuff over in Avro-land: https://issues.apache.org/jira/browse/AVRO-143.

          Show
          Jeff Hammerbacher added a comment - Chiming in to note that Doug is using ASM for some stuff over in Avro-land: https://issues.apache.org/jira/browse/AVRO-143 .
          Hide
          stack added a comment -

          Comments on doc:

          "Regions contain references to the coprocessor implementation classes associated with them."

          Q: On above, its indeed the classes, not objects? Objects can cross the split? Not easily anyways.

          Do we need both closing and pendingClose? I'd think just one? (Change pendingOpen to opening and keep closing ditching pendingClose). I don't know for sure. Just asking.

          Why no control over flush? Maybe it would want to hold up a flush? You think that too dangerous?

          On the interface:

          Drop the "H" from Coprocessor class I'd say. Thats the new fashion these days.

          It looks great.

          Rather, should we do the java Events model where one method gets all event types, the passed in object says that the event is. In the method, first thing you check if its an event you are interested in? Makes things easier to implement especially if you are only implementing part of the functionality. This model may not make sense though for this context or may be overkill (See java.util.EventObject and some of its implementations).

          Will Coprocessors make for lots of new object instantiations? Its going to be invoked on each Get and Scan.

          The logging interface seems odd. Why have new define? Why not just use apache logging?

          Should we be extracting an Interface from Region so we can have a Region implemetention and so your Coprocessor can have an implementation too? We sort of did something like with the "Incommon" interface we have for testing that has allows for implementations that run the same tests only now against the Region and then against the client-side. Extracting a 'official' Region interface sounds grand to me... would help with testing?

          How does the PrivateStore persist? Where? What you thinking?

          Looks great.

          Show
          stack added a comment - Comments on doc: "Regions contain references to the coprocessor implementation classes associated with them." Q: On above, its indeed the classes, not objects? Objects can cross the split? Not easily anyways. Do we need both closing and pendingClose? I'd think just one? (Change pendingOpen to opening and keep closing ditching pendingClose). I don't know for sure. Just asking. Why no control over flush? Maybe it would want to hold up a flush? You think that too dangerous? On the interface: Drop the "H" from Coprocessor class I'd say. Thats the new fashion these days. It looks great. Rather, should we do the java Events model where one method gets all event types, the passed in object says that the event is. In the method, first thing you check if its an event you are interested in? Makes things easier to implement especially if you are only implementing part of the functionality. This model may not make sense though for this context or may be overkill (See java.util.EventObject and some of its implementations). Will Coprocessors make for lots of new object instantiations? Its going to be invoked on each Get and Scan. The logging interface seems odd. Why have new define? Why not just use apache logging? Should we be extracting an Interface from Region so we can have a Region implemetention and so your Coprocessor can have an implementation too? We sort of did something like with the "Incommon" interface we have for testing that has allows for implementations that run the same tests only now against the Region and then against the client-side. Extracting a 'official' Region interface sounds grand to me... would help with testing? How does the PrivateStore persist? Where? What you thinking? Looks great.
          Hide
          Andrew Purtell added a comment -

          "Regions contain references to the coprocessor implementation classes associated with them."
          Q: On above, its indeed the classes, not objects? Objects can cross the split? Not easily anyways.

          When regions are split, new coprocessor object instances would be allocated on the daughters – one instance for each of the coprocessor classes listed in the region metadata – when they are opening and the coprocessor's onOpen method is invoked to give it a chance to initialize. Prior to this the parent would be informed of the impending split via an onSplit invocation, and when it closes its onClose method would be called so it can clean up. How to manage the split beyond this would be the problem of the coprocessor.

          Do we need both closing and pendingClose? [...]

          I found that state transition in the master code and copied it verbatim from a comment block. Actually coprocessors only go through three states: opening, open, closing.

          Why no control over flush? Maybe it would want to hold up a flush? You think that too dangerous?

          I do think that is too dangerous.

          Rather, should we do the java Events model where one method gets all event types, the passed in object says that the event is. In the method, first thing you check if its an event you are interested in? Makes things easier to implement especially if you are only implementing part of the functionality. This model may not make sense though for this context or may be overkill (See java.util.EventObject and some of its implementations).

          I thought about that and go back and forth. Explicit interface is also self-documenting while arcane gotchas can hide in event specific detail. There's also the notion of using ASM to weave in policy enforcement. That could be easier if each callback is its own well defined method. On the other hand there's a lot of foo()

          { super(); }

          crap for each callback that a coprocessor does not care about. My current thinking is the later does not outweigh the former.

          By the way, I am thinking about using ASM to weave in CPU and memory accounting and limit enforcement as a generic code safety policy regardless.

          Will Coprocessors make for lots of new object instantiations? Its going to be invoked on each Get and Scan.

          Not unless the coprocessor does it.

          The logging interface seems odd. Why have new define? Why not just use apache logging?

          The idea is no I/O outside of the interface is allowed. There will be an additional verification step at classload time, implemented with ASM, that checks against a whitelist. Making the whitelist to the extent possible a single interface is a simplifying choice.

          Should we be extracting an Interface from Region so we can have a Region implemetention and so your Coprocessor can have an implementation too? We sort of did something like with the "Incommon" interface we have for testing that has allows for implementations that run the same tests only now against the Region and then against the client-side. Extracting a 'official' Region interface sounds grand to me... would help with testing?

          That's a good idea. Should be a separate issue?

          How does the PrivateStore persist? Where? What you thinking?

          One PrivateStore for each coprocessor would persist as an HFile+log in the region's store. Would be cloned into daughters on split. Would get periodic compaction whenever the store is compacted. The general idea is to do something less than manage a real table in a way that hooks in naturally with store management. I gave it a table interface but it could be just a bag of KVs if supporting multiple column families in a single HFile+log is too much trouble.

          Show
          Andrew Purtell added a comment - "Regions contain references to the coprocessor implementation classes associated with them." Q: On above, its indeed the classes, not objects? Objects can cross the split? Not easily anyways. When regions are split, new coprocessor object instances would be allocated on the daughters – one instance for each of the coprocessor classes listed in the region metadata – when they are opening and the coprocessor's onOpen method is invoked to give it a chance to initialize. Prior to this the parent would be informed of the impending split via an onSplit invocation, and when it closes its onClose method would be called so it can clean up. How to manage the split beyond this would be the problem of the coprocessor. Do we need both closing and pendingClose? [...] I found that state transition in the master code and copied it verbatim from a comment block. Actually coprocessors only go through three states: opening, open, closing. Why no control over flush? Maybe it would want to hold up a flush? You think that too dangerous? I do think that is too dangerous. Rather, should we do the java Events model where one method gets all event types, the passed in object says that the event is. In the method, first thing you check if its an event you are interested in? Makes things easier to implement especially if you are only implementing part of the functionality. This model may not make sense though for this context or may be overkill (See java.util.EventObject and some of its implementations). I thought about that and go back and forth. Explicit interface is also self-documenting while arcane gotchas can hide in event specific detail. There's also the notion of using ASM to weave in policy enforcement. That could be easier if each callback is its own well defined method. On the other hand there's a lot of foo() { super(); } crap for each callback that a coprocessor does not care about. My current thinking is the later does not outweigh the former. By the way, I am thinking about using ASM to weave in CPU and memory accounting and limit enforcement as a generic code safety policy regardless. Will Coprocessors make for lots of new object instantiations? Its going to be invoked on each Get and Scan. Not unless the coprocessor does it. The logging interface seems odd. Why have new define? Why not just use apache logging? The idea is no I/O outside of the interface is allowed. There will be an additional verification step at classload time, implemented with ASM, that checks against a whitelist. Making the whitelist to the extent possible a single interface is a simplifying choice. Should we be extracting an Interface from Region so we can have a Region implemetention and so your Coprocessor can have an implementation too? We sort of did something like with the "Incommon" interface we have for testing that has allows for implementations that run the same tests only now against the Region and then against the client-side. Extracting a 'official' Region interface sounds grand to me... would help with testing? That's a good idea. Should be a separate issue? How does the PrivateStore persist? Where? What you thinking? One PrivateStore for each coprocessor would persist as an HFile+log in the region's store. Would be cloned into daughters on split. Would get periodic compaction whenever the store is compacted. The general idea is to do something less than manage a real table in a way that hooks in naturally with store management. I gave it a table interface but it could be just a bag of KVs if supporting multiple column families in a single HFile+log is too much trouble.
          Hide
          stack added a comment -

          Would suggest that if coprocessors only need three states, then just do those. Region state transition is up for review. Will probably change anyways.

          No worries on going the pedantic route – a method per rather than all into the one method with a handler switch. For sure makes things like introspection and autogeneration easier. Its just a pain for the human having to implement all methods though only interested in a few.

          I know nothing about ASM. Go for it.

          OK on they why you have a logging interface.

          Yes to separate issue for Region interface.

          First thought is that mimicing table interface might confuse more than it helps but need to study more before I can offer stronger opinion.

          Otherwise, all sounds great.

          Show
          stack added a comment - Would suggest that if coprocessors only need three states, then just do those. Region state transition is up for review. Will probably change anyways. No worries on going the pedantic route – a method per rather than all into the one method with a handler switch. For sure makes things like introspection and autogeneration easier. Its just a pain for the human having to implement all methods though only interested in a few. I know nothing about ASM. Go for it. OK on they why you have a logging interface. Yes to separate issue for Region interface. First thought is that mimicing table interface might confuse more than it helps but need to study more before I can offer stronger opinion. Otherwise, all sounds great.
          Hide
          Andrew Purtell added a comment -

          .bq Would suggest that if coprocessors only need three states, then just do those.

          I'll make that clear in the documentation.

          First thought is that mimicing table interface might confuse more than it helps but need to study more before I can offer stronger opinion.

          There are two table interfaces currently:
          1) PrivateStore – a full table, private to the Coprocessor instance, backed by a HFile+log
          2) RegionAccessor – a table interface to the region associated with the Coprocessor, will reject edits outside the region, otherwise allows (controlled) admin and mutation actions on the region. Also of note here the Coprocessor hooks will be called for its own edits, just makes things easier.

          Just FYI I have the environment implemented except for PrivateStore. No classloader or ASM bits yet; I need to study and think more about that.

          Meantime the next step is hooking up Coprocessors to the HRegion via calls out to the interface in all the right places in HRegion, and possibly a few up in HRegionServer.

          Show
          Andrew Purtell added a comment - .bq Would suggest that if coprocessors only need three states, then just do those. I'll make that clear in the documentation. First thought is that mimicing table interface might confuse more than it helps but need to study more before I can offer stronger opinion. There are two table interfaces currently: 1) PrivateStore – a full table, private to the Coprocessor instance, backed by a HFile+log 2) RegionAccessor – a table interface to the region associated with the Coprocessor, will reject edits outside the region, otherwise allows (controlled) admin and mutation actions on the region. Also of note here the Coprocessor hooks will be called for its own edits, just makes things easier. Just FYI I have the environment implemented except for PrivateStore. No classloader or ASM bits yet; I need to study and think more about that. Meantime the next step is hooking up Coprocessors to the HRegion via calls out to the interface in all the right places in HRegion, and possibly a few up in HRegionServer.
          Hide
          Andrew Purtell added a comment -

          Will redo how filters and coprocessors relate – Rather than incidentally contributing them, coprocessors should replace them.

          Show
          Andrew Purtell added a comment - Will redo how filters and coprocessors relate – Rather than incidentally contributing them, coprocessors should replace them.
          Hide
          stack added a comment -

          Are you using HTableInterface for your "table" implementations?

          +1 that coprocessors be used to implement filters. Would we keep the filter interface as is? With coprocessors, filters would be able to have state that crossed rows and even regions? (Currently state is in-row only).

          Critical I'd say is that coprocessors are testable without having to spin up servers. They should be testable standalone if possible.

          Show
          stack added a comment - Are you using HTableInterface for your "table" implementations? +1 that coprocessors be used to implement filters. Would we keep the filter interface as is? With coprocessors, filters would be able to have state that crossed rows and even regions? (Currently state is in-row only). Critical I'd say is that coprocessors are testable without having to spin up servers. They should be testable standalone if possible.
          Hide
          Andrew Purtell added a comment -

          Are you using HTableInterface for your "table" implementation?

          No, but I should you are right.

          Critical I'd say is that coprocessors are testable without having to spin up servers.

          It would depend on the coprocessor but for "builtins" like filters we can manage that.

          Of course to test the framework there needs to be a (sub)suite of tests which run against a live region server.

          Show
          Andrew Purtell added a comment - Are you using HTableInterface for your "table" implementation? No, but I should you are right. Critical I'd say is that coprocessors are testable without having to spin up servers. It would depend on the coprocessor but for "builtins" like filters we can manage that. Of course to test the framework there needs to be a (sub)suite of tests which run against a live region server.
          Hide
          Andrew Purtell added a comment -

          Are you using HTableInterface for your "table" implementation?

          No, but I should you are right.

          On second thought it should be an internal interface. Currently, a subset of HRegionInterface.

          Show
          Andrew Purtell added a comment - Are you using HTableInterface for your "table" implementation? No, but I should you are right. On second thought it should be an internal interface. Currently, a subset of HRegionInterface.
          Hide
          Andrew Purtell added a comment -

          Make various integration modes (region mediation, ioctl-like command target, filter implementation) their own interfaces. Determine at runtime what interfaces a coprocessor implements and hook up accordingly.

          Patch is a work in progress.

          Show
          Andrew Purtell added a comment - Make various integration modes (region mediation, ioctl-like command target, filter implementation) their own interfaces. Determine at runtime what interfaces a coprocessor implements and hook up accordingly. Patch is a work in progress.
          Hide
          Todd Lipcon added a comment -

          Does loading arbitrary Java code into the region servers not scare the crap out of anyone else?

          I'd feel way better about this if it were embedding a language that could be provably safe. For example, VMware's dtrace-alike (vprobes) uses an embedded language called "vp" which can put tight upper bounds on allocation, time, etc. See http://www.vmware.com/products/beta/ws/vprobes_reference.pdf . That approach is extra strict because it's running in the VM monitor which has to be super stable, but the same general idea should be applied to coprocessors IMHO.

          Show
          Todd Lipcon added a comment - Does loading arbitrary Java code into the region servers not scare the crap out of anyone else? I'd feel way better about this if it were embedding a language that could be provably safe. For example, VMware's dtrace-alike (vprobes) uses an embedded language called "vp" which can put tight upper bounds on allocation, time, etc. See http://www.vmware.com/products/beta/ws/vprobes_reference.pdf . That approach is extra strict because it's running in the VM monitor which has to be super stable, but the same general idea should be applied to coprocessors IMHO.
          Hide
          Andrew Purtell added a comment -

          Nobody has to develop and deploy custom coprocessors if they don't want the risk.

          Maybe it's not highlighted enough but my current thinking and plan is to use ASM to also impose some constraints. This is middle ground between arbitrary function and a locked down language and will take only reasonable effort to achieve (with part time one person contribution).

          Beyond that, I'm not opposed if someone wants to contribute a restricted yet somehow useful little language, I just have no plans to do that or I'll be working on this indefinitely.

          Show
          Andrew Purtell added a comment - Nobody has to develop and deploy custom coprocessors if they don't want the risk. Maybe it's not highlighted enough but my current thinking and plan is to use ASM to also impose some constraints. This is middle ground between arbitrary function and a locked down language and will take only reasonable effort to achieve (with part time one person contribution). Beyond that, I'm not opposed if someone wants to contribute a restricted yet somehow useful little language, I just have no plans to do that or I'll be working on this indefinitely.
          Hide
          Todd Lipcon added a comment -

          Fair enough. Don't want to discourage the current work

          Show
          Todd Lipcon added a comment - Fair enough. Don't want to discourage the current work
          Hide
          Andrew Purtell added a comment -

          @Todd: Just to be clear I don't disagree with you. It's a question of dividing the problem into manageable chunks. My plan is to put ASM in the class loading path for coprocessors from the outset. In the beginning it will serve little purpose. I hope from there to develop CPU and memory allocation limit policy enforcement via runtime code weaving, either myself or with assistance. I put up HBASE-2058 to make this more clear.

          Show
          Andrew Purtell added a comment - @Todd: Just to be clear I don't disagree with you. It's a question of dividing the problem into manageable chunks. My plan is to put ASM in the class loading path for coprocessors from the outset. In the beginning it will serve little purpose. I hope from there to develop CPU and memory allocation limit policy enforcement via runtime code weaving, either myself or with assistance. I put up HBASE-2058 to make this more clear.
          Hide
          Andrew Purtell added a comment -

          Updated attachment.

          • Dynamically install Filter implementations provided by a Coprocessor via o.a.h.io.WritableFactories so Get and Scan deserialization will work afterward.
          • Rebased against latest trunk.
          Show
          Andrew Purtell added a comment - Updated attachment. Dynamically install Filter implementations provided by a Coprocessor via o.a.h.io.WritableFactories so Get and Scan deserialization will work afterward. Rebased against latest trunk.
          Hide
          Andrew Purtell added a comment -

          Latest patch contains simple working unit tests for basic Coprocessor hooks and also RegionObserver interface hooks.

          Also, the initial implementation of an in-process MapReduce framework. Coprocessors can optionally implement a 'MapReduce' interface which clients will at some point be able to invoke concurrently on all regions of the table within the HRS processes. (Server side needs unit tests and testing; no client side yet.) Note this is not MapReduce on the table; this is MapReduce on each region, concurrently.

          In-process MapReduce is multithreaded. Concurrency of mappers and reducers is specified separately. Map jobs are submitted with a Scan object which defines the scope and any filters for a scanner which feeds mappers. Mappers can emit intermediate KeyValues to a collector for reduction or can get references to objects in the coprocessor's environment and perform operations on them, e.g. increment an AtomicLong, etc. Reducers will get KeyValues from map phase output ordered and grouped by key. Reducers also have access to objects in the coprocessor environment. Therefore one can implement MapReduce in a manner very similar to Hadoop's MR framework, or e.g. aggregating functions can use shared variables to avoid the overhead of generating (and processing) a lot of intermediates.

          An in-process MapReduce job can be configured to auto commit. If so, KeyValues written to the reduce collector by reducers will be automatically committed back to the region after all reducers have completed execution. Up until all mappers and reducers successfully complete execution no values are committed to the region. Then, we try really hard to commit them all.

          KeyValues emitted by reducers must have a row key that falls within the bounds of the region if the job is auto committing. Otherwise, the output can be arbitrary.

          If a job is not auto committing, when it completes clients have access to the KeyValues output by the reducer via a scanner like interface.

          The in-process MapReduce framework uses leases. A job is only alive as long as it has a lease. Its output KeyValues are only available as long as it has a lease. So for long running jobs the client must periodically poll status to keep it alive, and then retrieval by "scanner" will also renew the lease. A lease cannot expire during auto commit.

          Show
          Andrew Purtell added a comment - Latest patch contains simple working unit tests for basic Coprocessor hooks and also RegionObserver interface hooks. Also, the initial implementation of an in-process MapReduce framework. Coprocessors can optionally implement a 'MapReduce' interface which clients will at some point be able to invoke concurrently on all regions of the table within the HRS processes. (Server side needs unit tests and testing; no client side yet.) Note this is not MapReduce on the table; this is MapReduce on each region, concurrently. In-process MapReduce is multithreaded. Concurrency of mappers and reducers is specified separately. Map jobs are submitted with a Scan object which defines the scope and any filters for a scanner which feeds mappers. Mappers can emit intermediate KeyValues to a collector for reduction or can get references to objects in the coprocessor's environment and perform operations on them, e.g. increment an AtomicLong, etc. Reducers will get KeyValues from map phase output ordered and grouped by key. Reducers also have access to objects in the coprocessor environment. Therefore one can implement MapReduce in a manner very similar to Hadoop's MR framework, or e.g. aggregating functions can use shared variables to avoid the overhead of generating (and processing) a lot of intermediates. An in-process MapReduce job can be configured to auto commit. If so, KeyValues written to the reduce collector by reducers will be automatically committed back to the region after all reducers have completed execution. Up until all mappers and reducers successfully complete execution no values are committed to the region. Then, we try really hard to commit them all. KeyValues emitted by reducers must have a row key that falls within the bounds of the region if the job is auto committing. Otherwise, the output can be arbitrary. If a job is not auto committing, when it completes clients have access to the KeyValues output by the reducer via a scanner like interface. The in-process MapReduce framework uses leases. A job is only alive as long as it has a lease. Its output KeyValues are only available as long as it has a lease. So for long running jobs the client must periodically poll status to keep it alive, and then retrieval by "scanner" will also renew the lease. A lease cannot expire during auto commit.
          Hide
          stack added a comment -

          Woah!

          Show
          stack added a comment - Woah!
          Hide
          Andrew Purtell added a comment -

          Latest patch has working simple unit tests for MapReduce.

          There is also a working unit test for on-demand loading of coprocessor classes from jars referenced via table attributes:

          Tables can have any number of attributes with keys that start with "Coprocessor" and values of the form <path>:<class>:<priority>, e.g.

          'Coprocessor$1' => 'hdfs://localhost:8020/hbase/coprocessors/test.jar:Test:1000'
          'Coprocessor$2' => '/hbase/coprocessors/test2.jar:AnotherTest:1001'

          <path> must point to a jar, can be on any filesystem supported by the Hadoop FileSystem object.

          <class> is the coprocessor implementation class. A jar can contain more than one coprocessor implementation, but only one can be specified at a time in each table attribute.

          <priority> is an integer. Coprocessors are executed in order according to the natural ordering of the int. Coprocessors can optionally abort actions. So typically one would want to put authoritative CPs (security policy implementations, perhaps) ahead of observers.

          Show
          Andrew Purtell added a comment - Latest patch has working simple unit tests for MapReduce. There is also a working unit test for on-demand loading of coprocessor classes from jars referenced via table attributes: Tables can have any number of attributes with keys that start with "Coprocessor" and values of the form <path>:<class>:<priority>, e.g. 'Coprocessor$1' => 'hdfs://localhost:8020/hbase/coprocessors/test.jar:Test:1000' 'Coprocessor$2' => '/hbase/coprocessors/test2.jar:AnotherTest:1001' <path> must point to a jar, can be on any filesystem supported by the Hadoop FileSystem object. <class> is the coprocessor implementation class. A jar can contain more than one coprocessor implementation, but only one can be specified at a time in each table attribute. <priority> is an integer. Coprocessors are executed in order according to the natural ordering of the int. Coprocessors can optionally abort actions. So typically one would want to put authoritative CPs (security policy implementations, perhaps) ahead of observers.
          Hide
          Andrew Purtell added a comment -

          Added multithread MapReduce test, and correctness fixes.

          Show
          Andrew Purtell added a comment - Added multithread MapReduce test, and correctness fixes.
          Hide
          Karthik K added a comment -

          Andrew: Is there a (public) git repository available with the patch , from which we can pull from ?

          Show
          Karthik K added a comment - Andrew: Is there a (public) git repository available with the patch , from which we can pull from ?
          Hide
          Andrew Purtell added a comment -
          Show
          Andrew Purtell added a comment - As you wish: http://github.com/apurtell/hbase-coprocessor
          Hide
          Andrew Purtell added a comment -

          I merged up the patch to latest trunk and pushed it to github. It has been some time since I last ran the tests, they may currently fail. Will be fixing them up soon if so.

          Show
          Andrew Purtell added a comment - I merged up the patch to latest trunk and pushed it to github. It has been some time since I last ran the tests, they may currently fail. Will be fixing them up soon if so.
          Hide
          Andrew Purtell added a comment -

          HBASE-2001-RegionObserver.patch is just the parts of the 2001 patch for the RegionObserver interface, which enables extension of the regionserver through stacking dynamically loaded extensions on upcalls from HRegion. Made some improvements over the other patch. Added a test case.

          Show
          Andrew Purtell added a comment - HBASE-2001 -RegionObserver.patch is just the parts of the 2001 patch for the RegionObserver interface, which enables extension of the regionserver through stacking dynamically loaded extensions on upcalls from HRegion. Made some improvements over the other patch. Added a test case.
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/
          -----------------------------------------------------------

          Review request for hbase.

          Summary
          -------

          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.

          This addresses bug HBASE-2001.
          http://issues.apache.org/jira/browse/HBASE-2001

          Diffs


          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION

          Diff: http://review.hbase.org/r/96/diff

          Testing
          -------

          All the new unit tests plus TestHRegion pass locally.

          Thanks,

          Andrew

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/ ----------------------------------------------------------- Review request for hbase. Summary ------- 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. This addresses bug HBASE-2001 . http://issues.apache.org/jira/browse/HBASE-2001 Diffs src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 287cd48 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION Diff: http://review.hbase.org/r/96/diff Testing ------- All the new unit tests plus TestHRegion pass locally. Thanks, Andrew
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/
          -----------------------------------------------------------

          (Updated 2010-05-31 22:30:41.618893)

          Review request for hbase.

          Summary
          -------

          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.

          This addresses bug HBASE-2001.
          http://issues.apache.org/jira/browse/HBASE-2001

          Diffs


          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 2413e98
          src/main/java/org/apache/hadoop/hbase/regionserver/MinorCompactingStoreScanner.java 71f738e
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 515b42f
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION

          Diff: http://review.hbase.org/r/96/diff

          Testing
          -------

          All the new unit tests plus TestHRegion pass locally.

          Thanks,

          Andrew

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/ ----------------------------------------------------------- (Updated 2010-05-31 22:30:41.618893) Review request for hbase. Summary ------- 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. This addresses bug HBASE-2001 . http://issues.apache.org/jira/browse/HBASE-2001 Diffs src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 2413e98 src/main/java/org/apache/hadoop/hbase/regionserver/MinorCompactingStoreScanner.java 71f738e src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 515b42f src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION Diff: http://review.hbase.org/r/96/diff Testing ------- All the new unit tests plus TestHRegion pass locally. Thanks, Andrew
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/
          -----------------------------------------------------------

          (Updated 2010-05-31 22:47:25.650165)

          Review request for hbase.

          Summary
          -------

          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.

          This addresses bug HBASE-2001.
          http://issues.apache.org/jira/browse/HBASE-2001

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 2413e98
          src/main/java/org/apache/hadoop/hbase/regionserver/MinorCompactingStoreScanner.java 71f738e
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 515b42f
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION

          Diff: http://review.hbase.org/r/96/diff

          Testing
          -------

          All the new unit tests plus TestHRegion pass locally.

          Thanks,

          Andrew

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/ ----------------------------------------------------------- (Updated 2010-05-31 22:47:25.650165) Review request for hbase. Summary ------- 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. This addresses bug HBASE-2001 . http://issues.apache.org/jira/browse/HBASE-2001 Diffs (updated) src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 2413e98 src/main/java/org/apache/hadoop/hbase/regionserver/MinorCompactingStoreScanner.java 71f738e src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 515b42f src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION Diff: http://review.hbase.org/r/96/diff Testing ------- All the new unit tests plus TestHRegion pass locally. Thanks, Andrew
          Hide
          HBase Review Board added a comment -

          Message from: "Todd Lipcon" <todd@cloudera.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review121
          -----------------------------------------------------------

          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?

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment676>

          before or after reporting?

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment677>

          this map-like interface is somewhat confusing - what's the purpose of it?

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java
          <http://review.hbase.org/r/96/#comment678>

          woah, can we add something to the build to build this jar as a test resource, or something?

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          <http://review.hbase.org/r/96/#comment679>

          you could use mockito verification to do this, probably would be simpler

          • Todd
          Show
          HBase Review Board added a comment - Message from: "Todd Lipcon" <todd@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review121 ----------------------------------------------------------- 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? src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment676 > before or after reporting? src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment677 > this map-like interface is somewhat confusing - what's the purpose of it? src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java < http://review.hbase.org/r/96/#comment678 > woah, can we add something to the build to build this jar as a test resource, or something? src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java < http://review.hbase.org/r/96/#comment679 > you could use mockito verification to do this, probably would be simpler Todd
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          On 2010-06-02 17:24:34, Todd Lipcon wrote:

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

          This seems like a good idea when dev'ing an Interface. After the 3rd implemenation you'll have some confidence in your Interface.

          • stack

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review121
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: stack@duboce.net On 2010-06-02 17:24:34, Todd Lipcon wrote: > 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? This seems like a good idea when dev'ing an Interface. After the 3rd implemenation you'll have some confidence in your Interface. stack ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review121 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review123
          -----------------------------------------------------------

          This looks really great Andrew.

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment680>

          What about unloading? You remember that conversation up on irc of how loading is one thing but unloading w/o breakage is hard prob.

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment681>

          Good

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment682>

          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)

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment683>

          Great doc. Some could probably go out into package doc...

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment684>

          Make this javadoc?

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment685>

          Which table is this?

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment686>

          Needs to be Writable?

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.hbase.org/r/96/#comment687>

          This is a mixin you'd use if you want to be notified about compactions, etc.?

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.hbase.org/r/96/#comment688>

          What would this be used for? For CP to call out elsewhere on a table?

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.hbase.org/r/96/#comment689>

          Write this as LOG.warn("Failed close of table=" + table, e)?

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.hbase.org/r/96/#comment690>

          Is this needed?

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.hbase.org/r/96/#comment691>

          How does this get triggered inside the HRS? I suppose I'll learn later down in this patch.

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.hbase.org/r/96/#comment692>

          Its always on then?

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.hbase.org/r/96/#comment693>

          Who would want to get at this?

          • stack
          Show
          HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review123 ----------------------------------------------------------- This looks really great Andrew. src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment680 > What about unloading? You remember that conversation up on irc of how loading is one thing but unloading w/o breakage is hard prob. src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment681 > Good src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment682 > 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) src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment683 > Great doc. Some could probably go out into package doc... src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment684 > Make this javadoc? src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment685 > Which table is this? src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment686 > Needs to be Writable? src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.hbase.org/r/96/#comment687 > This is a mixin you'd use if you want to be notified about compactions, etc.? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.hbase.org/r/96/#comment688 > What would this be used for? For CP to call out elsewhere on a table? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.hbase.org/r/96/#comment689 > Write this as LOG.warn("Failed close of table=" + table, e)? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.hbase.org/r/96/#comment690 > Is this needed? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.hbase.org/r/96/#comment691 > How does this get triggered inside the HRS? I suppose I'll learn later down in this patch. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.hbase.org/r/96/#comment692 > Its always on then? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.hbase.org/r/96/#comment693 > Who would want to get at this? stack
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@facebook.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review124
          -----------------------------------------------------------

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment698>

          What is purpose of this?

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.hbase.org/r/96/#comment694>

          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.

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.hbase.org/r/96/#comment695>

          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.

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.hbase.org/r/96/#comment701>

          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?

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.hbase.org/r/96/#comment696>

          And descending timestamp

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.hbase.org/r/96/#comment697>

          This is great javadoc

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.hbase.org/r/96/#comment699>

          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.

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.hbase.org/r/96/#comment700>

          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?

          • Jonathan
          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@facebook.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review124 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment698 > What is purpose of this? src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.hbase.org/r/96/#comment694 > 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. src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.hbase.org/r/96/#comment695 > 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. src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.hbase.org/r/96/#comment701 > 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? src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.hbase.org/r/96/#comment696 > And descending timestamp src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.hbase.org/r/96/#comment697 > This is great javadoc src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.hbase.org/r/96/#comment699 > 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. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.hbase.org/r/96/#comment700 > 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? Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@facebook.com>

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          >

          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?

          • Jonathan

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review124
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@facebook.com> On 2010-06-03 11:19:47, Jonathan Gray wrote: > 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? Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review124 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-06-02 17:24:34, Todd Lipcon wrote:

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

          stack wrote:

          This seems like a good idea when dev'ing an Interface. After the 3rd implemenation you'll have some confidence in your Interface.

          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.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review121
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-06-02 17:24:34, Todd Lipcon wrote: > 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? stack wrote: This seems like a good idea when dev'ing an Interface. After the 3rd implemenation you'll have some confidence in your Interface. 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. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review121 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-06-02 17:24:34, Todd Lipcon wrote:

          > src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java, line 40

          > <http://review.hbase.org/r/96/diff/4/?file=898#file898line40>

          >

          > woah, can we add something to the build to build this jar as a test resource, or something?

          Yes.

          On 2010-06-02 17:24:34, Todd Lipcon wrote:

          > src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java, line 43

          > <http://review.hbase.org/r/96/diff/4/?file=899#file899line43>

          >

          > you could use mockito verification to do this, probably would be simpler

          Thanks this would be a good opportunity to learn mockito.

          On 2010-06-02 17:24:34, Todd Lipcon wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 149

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line149>

          >

          > this map-like interface is somewhat confusing - what's the purpose of it?

          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.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review121
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-06-02 17:24:34, Todd Lipcon wrote: > src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java, line 40 > < http://review.hbase.org/r/96/diff/4/?file=898#file898line40 > > > woah, can we add something to the build to build this jar as a test resource, or something? Yes. On 2010-06-02 17:24:34, Todd Lipcon wrote: > src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java, line 43 > < http://review.hbase.org/r/96/diff/4/?file=899#file899line43 > > > you could use mockito verification to do this, probably would be simpler Thanks this would be a good opportunity to learn mockito. On 2010-06-02 17:24:34, Todd Lipcon wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 149 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line149 > > > this map-like interface is somewhat confusing - what's the purpose of it? 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. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review121 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 140

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line140>

          >

          > Which table is this?

          Any table. The idea is the coprocessor can create any private tables it needs to implement its functionality.

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 27

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line27>

          >

          > What about unloading? You remember that conversation up on irc of how loading is one thing but unloading w/o breakage is hard prob.

          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.

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 150

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line150>

          >

          > Needs to be Writable?

          No.

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 29

          > <http://review.hbase.org/r/96/diff/4/?file=893#file893line29>

          >

          > This is a mixin you'd use if you want to be notified about compactions, etc.?

          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.

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 85

          > <http://review.hbase.org/r/96/diff/4/?file=894#file894line85>

          >

          > What would this be used for? For CP to call out elsewhere on a table?

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

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 237

          > <http://review.hbase.org/r/96/diff/4/?file=894#file894line237>

          >

          > Is this needed?

          Why not.

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 245

          > <http://review.hbase.org/r/96/diff/4/?file=895#file895line245>

          >

          > Its always on then?

          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.

          On 2010-06-02 23:28:50, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2885

          > <http://review.hbase.org/r/96/diff/4/?file=895#file895line2885>

          >

          > Who would want to get at this?

          Tests. So probably this does not have to be public.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review123
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 140 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line140 > > > Which table is this? Any table. The idea is the coprocessor can create any private tables it needs to implement its functionality. On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 27 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line27 > > > What about unloading? You remember that conversation up on irc of how loading is one thing but unloading w/o breakage is hard prob. 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 . On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 150 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line150 > > > Needs to be Writable? No. On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 29 > < http://review.hbase.org/r/96/diff/4/?file=893#file893line29 > > > This is a mixin you'd use if you want to be notified about compactions, etc.? 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. On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 85 > < http://review.hbase.org/r/96/diff/4/?file=894#file894line85 > > > What would this be used for? For CP to call out elsewhere on a table? 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). On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 237 > < http://review.hbase.org/r/96/diff/4/?file=894#file894line237 > > > Is this needed? Why not. On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 245 > < http://review.hbase.org/r/96/diff/4/?file=895#file895line245 > > > Its always on then? 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. On 2010-06-02 23:28:50, stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2885 > < http://review.hbase.org/r/96/diff/4/?file=895#file895line2885 > > > Who would want to get at this? Tests. So probably this does not have to be public. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review123 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-06-02 17:24:34, Todd Lipcon wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 53

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line53>

          >

          > before or after reporting?

          Should be just before. Will double check.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review121
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-06-02 17:24:34, Todd Lipcon wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 53 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line53 > > > before or after reporting? Should be just before. Will double check. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review121 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 131

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line131>

          >

          > What is purpose of this?

          Coprocessors may want to change their behavior based on different minor versions.

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 189

          > <http://review.hbase.org/r/96/diff/4/?file=892#file892line189>

          >

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

          Then this signal should shift to the flush upcall.

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 32

          > <http://review.hbase.org/r/96/diff/4/?file=893#file893line32>

          >

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

          RegionObserver used to have a cleaner distinction from Coprocessor. Initially Coprocessor hooked internal region housekeeping; meanwhile RegionObserver wrapped HRegionInterface.

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 33

          > <http://review.hbase.org/r/96/diff/4/?file=893#file893line33>

          >

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

          An interesting idea to consider renaming the interfaces for clarity.

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 49

          > <http://review.hbase.org/r/96/diff/4/?file=893#file893line49>

          >

          > And descending timestamp

          Got it.

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 87

          > <http://review.hbase.org/r/96/diff/4/?file=893#file893line87>

          >

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

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

          On 2010-06-03 11:19:47, Jonathan Gray wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 694

          > <http://review.hbase.org/r/96/diff/4/?file=895#file895line694>

          >

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

          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.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/96/#review124
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 131 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line131 > > > What is purpose of this? Coprocessors may want to change their behavior based on different minor versions. On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java, line 189 > < http://review.hbase.org/r/96/diff/4/?file=892#file892line189 > > > 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. Then this signal should shift to the flush upcall. On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 32 > < http://review.hbase.org/r/96/diff/4/?file=893#file893line32 > > > 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. RegionObserver used to have a cleaner distinction from Coprocessor. Initially Coprocessor hooked internal region housekeeping; meanwhile RegionObserver wrapped HRegionInterface. On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 33 > < http://review.hbase.org/r/96/diff/4/?file=893#file893line33 > > > 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? An interesting idea to consider renaming the interfaces for clarity. On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 49 > < http://review.hbase.org/r/96/diff/4/?file=893#file893line49 > > > And descending timestamp Got it. On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java, line 87 > < http://review.hbase.org/r/96/diff/4/?file=893#file893line87 > > > 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. 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". On 2010-06-03 11:19:47, Jonathan Gray wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 694 > < http://review.hbase.org/r/96/diff/4/?file=895#file895line694 > > > 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? 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. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/96/#review124 -----------------------------------------------------------
          Hide
          Eugene Koontz added a comment -

          One of the tests for Coprocessors, (src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java), needs to be fixed in light of HBASE-2461, because currently TestCoprocessorInterface tries to call the HRegion::splitRegion() method, which no longer exists as part of fix for HBASE-2461.

          (HBASE-2461 fix patch is here: https://issues.apache.org/jira/secure/attachment/12451153/2461-v8.txt )

          Show
          Eugene Koontz added a comment - One of the tests for Coprocessors, (src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java), needs to be fixed in light of HBASE-2461 , because currently TestCoprocessorInterface tries to call the HRegion::splitRegion() method, which no longer exists as part of fix for HBASE-2461 . ( HBASE-2461 fix patch is here: https://issues.apache.org/jira/secure/attachment/12451153/2461-v8.txt )
          Hide
          Jonathan Gray added a comment -

          @Andy, are you or anyone else at TM working on this right now? There's some things I'd like to start experimenting with using coprocessors and I think this patch was pretty close to commitable.

          Also, I would definitely be interested in a pre-Get hook in addition to the post-Get hook, as this would be required for one use case I'm considering.

          Show
          Jonathan Gray added a comment - @Andy, are you or anyone else at TM working on this right now? There's some things I'd like to start experimenting with using coprocessors and I think this patch was pretty close to commitable. Also, I would definitely be interested in a pre-Get hook in addition to the post-Get hook, as this would be required for one use case I'm considering.
          Hide
          Andrew Purtell added a comment -

          Yes. I'll follow up with you privately.

          Show
          Andrew Purtell added a comment - Yes. I'll follow up with you privately.
          Hide
          Mingjie Lai added a comment -

          HBASE-2001-RegionObserver-2.patch is merged with the latest trunk (with master-write merge: 2692). In addition, with some minor improves.

          We remove map/reduce interface/job from the patch. Right now we're working towards implementing role based access control by CP, and we only need RegionObserver interface for it. RBAC would be the first realistic application utilizing CP.

          @Jonathan, we don't distinguish preGet() or postGet() right now. onGet() is actually a postGet(). If there is a valid use case, we can definitely add the hooks.

          Show
          Mingjie Lai added a comment - HBASE-2001 -RegionObserver-2.patch is merged with the latest trunk (with master-write merge: 2692). In addition, with some minor improves. We remove map/reduce interface/job from the patch. Right now we're working towards implementing role based access control by CP, and we only need RegionObserver interface for it. RBAC would be the first realistic application utilizing CP. @Jonathan, we don't distinguish preGet() or postGet() right now. onGet() is actually a postGet(). If there is a valid use case, we can definitely add the hooks.
          Hide
          Andrew Purtell added a comment -

          We need preX hooks to make authoritative decisions, so will split them all into preX and postX. For implementation of security policies, the preX hooks will be most appropriate to override. For other applications it will depend if input or output or both should be intercepted.

          Show
          Andrew Purtell added a comment - We need preX hooks to make authoritative decisions, so will split them all into preX and postX. For implementation of security policies, the preX hooks will be most appropriate to override. For other applications it will depend if input or output or both should be intercepted.
          Hide
          Jonathan Gray added a comment -

          +1 on having pre and post for everything. More clear naming too.

          Show
          Jonathan Gray added a comment - +1 on having pre and post for everything. More clear naming too.
          Hide
          HBase Review Board added a comment -

          Message from: "Mingjie Lai" <mjlai09@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/
          -----------------------------------------------------------

          Review request for hbase.

          Summary
          -------

          The diff actually contains 2 seperate patches: HBase-2001 and the one for (HBASE-2002+HBASE-2321). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002, HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch.

          Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor:

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
          src/main/resources/hbase-default.xml
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java

          ==========================

          (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.)

          Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader.

          Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are:

          • Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations.
          • RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc.
          • CommandTarget: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server.

          Coprocessor:
          A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally.

          Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly.

          RegionObserver
          If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region.

          CommandTarget:
          Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points.

          Through CommandTarget and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can specify new passed parameters and return types for a method. And the new CommandTarget methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...).

          Coprocess loading
          A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table.

          (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading)

          This addresses bug HBase-2001.
          http://issues.apache.org/jira/browse/HBase-2001

          Diffs


          src/main/java/org/apache/hadoop/hbase/client/Action.java 556ea81
          src/main/java/org/apache/hadoop/hbase/client/Exec.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/ExecResult.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/HConnection.java 65f7618
          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java fbdec0b
          src/main/java/org/apache/hadoop/hbase/client/HTable.java 0dbf263
          src/main/java/org/apache/hadoop/hbase/client/MultiAction.java c6ea838
          src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 91bd04b
          src/main/java/org/apache/hadoop/hbase/client/RowRange.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/Scan.java 29b3cb0
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 83f623d
          src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 2b5eeb6
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java e23a629
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java e4c356d
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java a4810a6
          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java bccdc0e
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java fdef130
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java f2e4e7c
          src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java bb3b382
          src/main/resources/hbase-default.xml 5452fd1
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java PRE-CREATION

          Diff: http://review.cloudera.org/r/876/diff

          Testing
          -------

          Thanks,

          Mingjie

          Show
          HBase Review Board added a comment - Message from: "Mingjie Lai" <mjlai09@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/ ----------------------------------------------------------- Review request for hbase. Summary ------- The diff actually contains 2 seperate patches: HBase-2001 and the one for ( HBASE-2002 + HBASE-2321 ). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002 , HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch. Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor: src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java src/main/resources/hbase-default.xml src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java ========================== (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.) Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader. Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are: Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations. RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc. CommandTarget: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server. Coprocessor: A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally. Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly. RegionObserver If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region. CommandTarget: Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points. Through CommandTarget and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can specify new passed parameters and return types for a method. And the new CommandTarget methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...). Coprocess loading A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table. (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading) This addresses bug HBase-2001. http://issues.apache.org/jira/browse/HBase-2001 Diffs src/main/java/org/apache/hadoop/hbase/client/Action.java 556ea81 src/main/java/org/apache/hadoop/hbase/client/Exec.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ExecResult.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HConnection.java 65f7618 src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java fbdec0b src/main/java/org/apache/hadoop/hbase/client/HTable.java 0dbf263 src/main/java/org/apache/hadoop/hbase/client/MultiAction.java c6ea838 src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 91bd04b src/main/java/org/apache/hadoop/hbase/client/RowRange.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/Scan.java 29b3cb0 src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 83f623d src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 2b5eeb6 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java e23a629 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java e4c356d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java a4810a6 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/HMaster.java bccdc0e src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java fdef130 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java f2e4e7c src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java bb3b382 src/main/resources/hbase-default.xml 5452fd1 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java PRE-CREATION Diff: http://review.cloudera.org/r/876/diff Testing ------- Thanks, Mingjie
          Hide
          Mingjie Lai added a comment -

          The package-info file into HTML format. Please help to review and provide your feedback for the usage.

          Show
          Mingjie Lai added a comment - The package-info file into HTML format. Please help to review and provide your feedback for the usage.
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1323
          -----------------------------------------------------------

          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          <http://review.cloudera.org/r/876/#comment4433>

          Please add javadoc that explains that coprocessors loaded from configuration are all installed at System priority, chained in the order they appear in the value. (So users must explicitly order authoritative extensions before observers.)

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4462>

          If this throws a CoprocessorException the read lock won't be unlocked. Move out of the finally block.

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4463>

          This is not really post flush. Should be moved after internalFlushcache()

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4464>

          What happened to checkResources()?

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4466>

          Spelling error

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4467>

          Why not? What is excluded?

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4465>

          Spelling error

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4468>

          Yes we do. The coprocessor may be intercepting gets and performing global substitutions. Not allowing it to do so in this case would introduce inconsistency.

          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java
          <http://review.cloudera.org/r/876/#comment4469>

          What are these magic numbers?

          • Andrew
          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1323 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java < http://review.cloudera.org/r/876/#comment4433 > Please add javadoc that explains that coprocessors loaded from configuration are all installed at System priority, chained in the order they appear in the value. (So users must explicitly order authoritative extensions before observers.) src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4462 > If this throws a CoprocessorException the read lock won't be unlocked. Move out of the finally block. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4463 > This is not really post flush. Should be moved after internalFlushcache() src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4464 > What happened to checkResources()? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4466 > Spelling error src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4467 > Why not? What is excluded? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4465 > Spelling error src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4468 > Yes we do. The coprocessor may be intercepting gets and performing global substitutions. Not allowing it to do so in this case would introduce inconsistency. src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java < http://review.cloudera.org/r/876/#comment4469 > What are these magic numbers? Andrew
          Hide
          HBase Review Board added a comment -

          Message from: "Jonathan Gray" <jgray@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1366
          -----------------------------------------------------------

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4522>

          I would actually be -1 on having this call the get coprocessor hooks. Expected behavior (for me) would be that coprocessors hook into the client-side calls, not internal calls. For checkAndPut, are those Gets also wrapped by coprocessors?

          I suppose you could make an argument either way, but I'd err on the side of coprocessors attaching on client operations not internal ones.

          • Jonathan
          Show
          HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1366 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4522 > I would actually be -1 on having this call the get coprocessor hooks. Expected behavior (for me) would be that coprocessors hook into the client-side calls, not internal calls. For checkAndPut, are those Gets also wrapped by coprocessors? I suppose you could make an argument either way, but I'd err on the side of coprocessors attaching on client operations not internal ones. Jonathan
          Hide
          HBase Review Board added a comment -

          Message from: "Mingjie Lai" <mjlai09@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/
          -----------------------------------------------------------

          (Updated 2010-09-30 19:18:18.297399)

          Review request for hbase.

          Changes
          -------

          Fixes by review board comments

          Summary
          -------

          The diff actually contains 2 seperate patches: HBase-2001 and the one for (HBASE-2002+HBASE-2321). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002, HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch.

          Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor:

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
          src/main/resources/hbase-default.xml
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java

          ==========================

          (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.)

          Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader.

          Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are:

          • Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations.
          • RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc.
          • CommandTarget: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server.

          Coprocessor:
          A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally.

          Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly.

          RegionObserver
          If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region.

          CommandTarget:
          Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points.

          Through CommandTarget and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can specify new passed parameters and return types for a method. And the new CommandTarget methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...).

          Coprocess loading
          A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table.

          (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading)

          This addresses bug HBase-2001.
          http://issues.apache.org/jira/browse/HBase-2001

          Diffs (updated)


          conf/hbase-policy.xml PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java 365e4b9
          src/main/java/org/apache/hadoop/hbase/HServerAddress.java 3859968
          src/main/java/org/apache/hadoop/hbase/HServerInfo.java aaf4835
          src/main/java/org/apache/hadoop/hbase/client/Action.java 556ea81
          src/main/java/org/apache/hadoop/hbase/client/Batch.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/Exec.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/ExecResult.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/HConnection.java 65f7618
          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java fbdec0b
          src/main/java/org/apache/hadoop/hbase/client/HTable.java 0dbf263
          src/main/java/org/apache/hadoop/hbase/client/MultiAction.java c6ea838
          src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 91bd04b
          src/main/java/org/apache/hadoop/hbase/client/RowRange.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/Scan.java 29b3cb0
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 83f623d
          src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java 3a429c0
          src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 2b5eeb6
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java e23a629
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 19dbf2b
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java e4c356d
          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java bacf8b1
          src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java 71a0447
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java ee5dd8f
          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java fb1e834
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 0a4fbce
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e80ece8
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 4ba63d8
          src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 9aaf7c3
          src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java 01475d5
          src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java PRE-CREATION
          src/main/resources/hbase-default.xml 5fafe65
          src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java e5b1a30
          src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 63a6956
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 377e6b1
          src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java dc55407

          Diff: http://review.cloudera.org/r/876/diff

          Testing
          -------

          Thanks,

          Mingjie

          Show
          HBase Review Board added a comment - Message from: "Mingjie Lai" <mjlai09@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/ ----------------------------------------------------------- (Updated 2010-09-30 19:18:18.297399) Review request for hbase. Changes ------- Fixes by review board comments Summary ------- The diff actually contains 2 seperate patches: HBase-2001 and the one for ( HBASE-2002 + HBASE-2321 ). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002 , HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch. Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor: src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java src/main/resources/hbase-default.xml src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java ========================== (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.) Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader. Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are: Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations. RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc. CommandTarget: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server. Coprocessor: A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally. Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly. RegionObserver If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region. CommandTarget: Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points. Through CommandTarget and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can specify new passed parameters and return types for a method. And the new CommandTarget methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...). Coprocess loading A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table. (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading) This addresses bug HBase-2001. http://issues.apache.org/jira/browse/HBase-2001 Diffs (updated) conf/hbase-policy.xml PRE-CREATION src/main/java/org/apache/hadoop/hbase/HColumnDescriptor.java 365e4b9 src/main/java/org/apache/hadoop/hbase/HServerAddress.java 3859968 src/main/java/org/apache/hadoop/hbase/HServerInfo.java aaf4835 src/main/java/org/apache/hadoop/hbase/client/Action.java 556ea81 src/main/java/org/apache/hadoop/hbase/client/Batch.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/Exec.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/ExecResult.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/HConnection.java 65f7618 src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java fbdec0b src/main/java/org/apache/hadoop/hbase/client/HTable.java 0dbf263 src/main/java/org/apache/hadoop/hbase/client/MultiAction.java c6ea838 src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 91bd04b src/main/java/org/apache/hadoop/hbase/client/RowRange.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/Scan.java 29b3cb0 src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 83f623d src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java 3a429c0 src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 2b5eeb6 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java e23a629 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 19dbf2b src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java e4c356d src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java bacf8b1 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java 71a0447 src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java ee5dd8f src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/Status.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/HMaster.java fb1e834 src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 0a4fbce src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e80ece8 src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 4ba63d8 src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 9aaf7c3 src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java 01475d5 src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java PRE-CREATION src/main/resources/hbase-default.xml 5fafe65 src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java e5b1a30 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 63a6956 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassloading.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCommandTarget.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 377e6b1 src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java dc55407 Diff: http://review.cloudera.org/r/876/diff Testing ------- Thanks, Mingjie
          Hide
          Mingjie Lai added a comment -

          Modified package description file which reflect interface changes.

          Show
          Mingjie Lai added a comment - Modified package description file which reflect interface changes.
          Hide
          HBase Review Board added a comment -

          Message from: "Lars Francke" <lars.francke@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1380
          -----------------------------------------------------------

          Sorry for all the whitespace comments
          There are a bunch more in the test classes.

          src/main/java/org/apache/hadoop/hbase/HServerInfo.java
          <http://review.cloudera.org/r/876/#comment4581>

          The ternary operator does not need braces.

          src/main/java/org/apache/hadoop/hbase/client/Action.java
          <http://review.cloudera.org/r/876/#comment4582>

          The ternary operator does not need braces.

          src/main/java/org/apache/hadoop/hbase/client/Batch.java
          <http://review.cloudera.org/r/876/#comment4583>

          Remove extra character(s)

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          <http://review.cloudera.org/r/876/#comment4586>

          Should be of Type List<R> not ArrayList<R>

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          <http://review.cloudera.org/r/876/#comment4585>

          Why is this necessary? You already set the size by using the correct constructor.

          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          <http://review.cloudera.org/r/876/#comment4588>

          Remove the "public", interfaces don't need that.

          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          <http://review.cloudera.org/r/876/#comment4589>

          Remove the "public", interfaces don't need that.

          Also byte[] key in Map so every implementor has to make sure to use a Map that does this correctly.

          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          <http://review.cloudera.org/r/876/#comment4590>

          Remove the "public", interfaces don't need that.

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java
          <http://review.cloudera.org/r/876/#comment4591>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java
          <http://review.cloudera.org/r/876/#comment4592>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java
          <http://review.cloudera.org/r/876/#comment4593>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4594>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4595>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4596>

          Inconsistent formatting

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4597>

          Inconsistent formatting

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4598>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4599>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4600>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4601>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4602>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4603>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4604>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          <http://review.cloudera.org/r/876/#comment4605>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4612>

          Remove public static final

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4606>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4613>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4614>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4615>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4607>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4616>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4617>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4608>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4618>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4609>

          Whitespace stuff

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4610>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4619>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4611>

          Whitespace stuff

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4620>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          <http://review.cloudera.org/r/876/#comment4621>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4622>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4623>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4624>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4625>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4626>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4627>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4628>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          <http://review.cloudera.org/r/876/#comment4629>

          Remove public

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java
          <http://review.cloudera.org/r/876/#comment4630>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java
          <http://review.cloudera.org/r/876/#comment4631>

          Remove extra space behind the brace

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4642>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4632>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4643>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4644>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4645>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4633>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4646>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4634>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4647>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4648>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4649>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4635>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4650>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4651>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4636>

          Whitespace stuff

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4637>

          Whitespace stuff

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4638>

          Whitespace stuff

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4639>

          Whitespace stuff

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4652>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4653>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4654>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4655>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4656>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4640>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4657>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4658>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4641>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          <http://review.cloudera.org/r/876/#comment4659>

          Remove "public"

          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          <http://review.cloudera.org/r/876/#comment4707>

          Lots of whitespace stuff in this file. I've not listed them all.

          src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java
          <http://review.cloudera.org/r/876/#comment4708>

          public Log?

          src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
          <http://review.cloudera.org/r/876/#comment4709>

          private static final

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java
          <http://review.cloudera.org/r/876/#comment4710>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          <http://review.cloudera.org/r/876/#comment4711>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          <http://review.cloudera.org/r/876/#comment4712>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          <http://review.cloudera.org/r/876/#comment4713>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java
          <http://review.cloudera.org/r/876/#comment4714>

          Wrong formatting

          src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
          <http://review.cloudera.org/r/876/#comment4715>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/Status.java
          <http://review.cloudera.org/r/876/#comment4716>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4717>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4718>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4719>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4720>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4721>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4722>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          <http://review.cloudera.org/r/876/#comment4723>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4724>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4725>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4726>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4727>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4728>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4729>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4730>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4731>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4732>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4733>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4734>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4735>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4736>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4739>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4737>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4740>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4741>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4742>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4743>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4745>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4744>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4746>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4747>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4748>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4749>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4750>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4751>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4752>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4753>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4754>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4755>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4756>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4757>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4758>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4759>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4761>

          Inconsistent indentation with most of the rest of the file

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4760>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4762>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4763>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4764>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4765>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4766>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4768>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4767>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4770>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4771>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4773>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4772>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4774>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4775>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4776>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4777>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4778>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4779>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4780>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4781>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment4782>

          Lock should be acquired before the try block

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4783>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4784>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4785>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4786>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4787>

          Whitespace stuff and no comment for this parameter

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4788>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4789>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4790>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4791>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          <http://review.cloudera.org/r/876/#comment4792>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          <http://review.cloudera.org/r/876/#comment4793>

          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          <http://review.cloudera.org/r/876/#comment4794>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java
          <http://review.cloudera.org/r/876/#comment4795>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java
          <http://review.cloudera.org/r/876/#comment4796>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java
          <http://review.cloudera.org/r/876/#comment4797>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java
          <http://review.cloudera.org/r/876/#comment4798>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java
          <http://review.cloudera.org/r/876/#comment4799>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java
          <http://review.cloudera.org/r/876/#comment4800>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java
          <http://review.cloudera.org/r/876/#comment4801>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java
          <http://review.cloudera.org/r/876/#comment4802>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java
          <http://review.cloudera.org/r/876/#comment4803>

          Whitespace stuff

          src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java
          <http://review.cloudera.org/r/876/#comment4805>

          Unnecessary, comments are automatically inherited

          src/main/resources/hbase-default.xml
          <http://review.cloudera.org/r/876/#comment4806>

          Whitespace stuff

          src/main/resources/hbase-default.xml
          <http://review.cloudera.org/r/876/#comment4807>

          Whitespace stuff

          src/main/resources/hbase-default.xml
          <http://review.cloudera.org/r/876/#comment4808>

          Whitespace stuff

          src/main/resources/hbase-default.xml
          <http://review.cloudera.org/r/876/#comment4810>

          Whitespace stuff

          src/main/resources/hbase-default.xml
          <http://review.cloudera.org/r/876/#comment4811>

          Whitespace stuff

          • Lars
          Show
          HBase Review Board added a comment - Message from: "Lars Francke" <lars.francke@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1380 ----------------------------------------------------------- Sorry for all the whitespace comments There are a bunch more in the test classes. src/main/java/org/apache/hadoop/hbase/HServerInfo.java < http://review.cloudera.org/r/876/#comment4581 > The ternary operator does not need braces. src/main/java/org/apache/hadoop/hbase/client/Action.java < http://review.cloudera.org/r/876/#comment4582 > The ternary operator does not need braces. src/main/java/org/apache/hadoop/hbase/client/Batch.java < http://review.cloudera.org/r/876/#comment4583 > Remove extra character(s) src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < http://review.cloudera.org/r/876/#comment4586 > Should be of Type List<R> not ArrayList<R> src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < http://review.cloudera.org/r/876/#comment4585 > Why is this necessary? You already set the size by using the correct constructor. src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java < http://review.cloudera.org/r/876/#comment4588 > Remove the "public", interfaces don't need that. src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java < http://review.cloudera.org/r/876/#comment4589 > Remove the "public", interfaces don't need that. Also byte[] key in Map so every implementor has to make sure to use a Map that does this correctly. src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java < http://review.cloudera.org/r/876/#comment4590 > Remove the "public", interfaces don't need that. src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java < http://review.cloudera.org/r/876/#comment4591 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java < http://review.cloudera.org/r/876/#comment4592 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseCommandTarget.java < http://review.cloudera.org/r/876/#comment4593 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4594 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4595 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4596 > Inconsistent formatting src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4597 > Inconsistent formatting src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4598 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4599 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4600 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4601 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4602 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4603 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4604 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java < http://review.cloudera.org/r/876/#comment4605 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4612 > Remove public static final src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4606 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4613 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4614 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4615 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4607 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4616 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4617 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4608 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4618 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4609 > Whitespace stuff Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4610 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4619 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4611 > Whitespace stuff Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4620 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java < http://review.cloudera.org/r/876/#comment4621 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4622 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4623 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4624 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4625 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4626 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4627 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4628 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java < http://review.cloudera.org/r/876/#comment4629 > Remove public src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java < http://review.cloudera.org/r/876/#comment4630 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java < http://review.cloudera.org/r/876/#comment4631 > Remove extra space behind the brace src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4642 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4632 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4643 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4644 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4645 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4633 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4646 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4634 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4647 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4648 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4649 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4635 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4650 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4651 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4636 > Whitespace stuff Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4637 > Whitespace stuff Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4638 > Whitespace stuff Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4639 > Whitespace stuff Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4652 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4653 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4654 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4655 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4656 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4640 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4657 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4658 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4641 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < http://review.cloudera.org/r/876/#comment4659 > Remove "public" src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java < http://review.cloudera.org/r/876/#comment4707 > Lots of whitespace stuff in this file. I've not listed them all. src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java < http://review.cloudera.org/r/876/#comment4708 > public Log? src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java < http://review.cloudera.org/r/876/#comment4709 > private static final src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java < http://review.cloudera.org/r/876/#comment4710 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < http://review.cloudera.org/r/876/#comment4711 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < http://review.cloudera.org/r/876/#comment4712 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java < http://review.cloudera.org/r/876/#comment4713 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java < http://review.cloudera.org/r/876/#comment4714 > Wrong formatting src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java < http://review.cloudera.org/r/876/#comment4715 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/Status.java < http://review.cloudera.org/r/876/#comment4716 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4717 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4718 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4719 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4720 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4721 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4722 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java < http://review.cloudera.org/r/876/#comment4723 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4724 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4725 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4726 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4727 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4728 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4729 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4730 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4731 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4732 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4733 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4734 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4735 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4736 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4739 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4737 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4740 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4741 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4742 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4743 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4745 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4744 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4746 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4747 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4748 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4749 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4750 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4751 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4752 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4753 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4754 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4755 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4756 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4757 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4758 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4759 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4761 > Inconsistent indentation with most of the rest of the file src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4760 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4762 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4763 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4764 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4765 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4766 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4768 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4767 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4770 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4771 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4773 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4772 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4774 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4775 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4776 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4777 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4778 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4779 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4780 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4781 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment4782 > Lock should be acquired before the try block src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4783 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4784 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4785 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4786 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4787 > Whitespace stuff and no comment for this parameter src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4788 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4789 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4790 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4791 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < http://review.cloudera.org/r/876/#comment4792 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < http://review.cloudera.org/r/876/#comment4793 > src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java < http://review.cloudera.org/r/876/#comment4794 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java < http://review.cloudera.org/r/876/#comment4795 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java < http://review.cloudera.org/r/876/#comment4796 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java < http://review.cloudera.org/r/876/#comment4797 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java < http://review.cloudera.org/r/876/#comment4798 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java < http://review.cloudera.org/r/876/#comment4799 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java < http://review.cloudera.org/r/876/#comment4800 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java < http://review.cloudera.org/r/876/#comment4801 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java < http://review.cloudera.org/r/876/#comment4802 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java < http://review.cloudera.org/r/876/#comment4803 > Whitespace stuff src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java < http://review.cloudera.org/r/876/#comment4805 > Unnecessary, comments are automatically inherited src/main/resources/hbase-default.xml < http://review.cloudera.org/r/876/#comment4806 > Whitespace stuff src/main/resources/hbase-default.xml < http://review.cloudera.org/r/876/#comment4807 > Whitespace stuff src/main/resources/hbase-default.xml < http://review.cloudera.org/r/876/#comment4808 > Whitespace stuff src/main/resources/hbase-default.xml < http://review.cloudera.org/r/876/#comment4810 > Whitespace stuff src/main/resources/hbase-default.xml < http://review.cloudera.org/r/876/#comment4811 > Whitespace stuff Lars
          Hide
          stack added a comment -

          Moving into 0.92 as per discussion up on dev list (Correct me if I'm making wrong move here lads).

          Show
          stack added a comment - Moving into 0.92 as per discussion up on dev list (Correct me if I'm making wrong move here lads).
          Hide
          HBase Review Board added a comment -

          Message from: "Himanshu Vashishtha" <vashishtha.h@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1639
          -----------------------------------------------------------

          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          <http://review.cloudera.org/r/876/#comment5505>

          Is this list is for scalability. I am not able to visualise a scenario where a Cp has reference to more than one table. Given a region has only one CpHost instance, and though it can a multi CpImpls in it, but each of them will have its own Environment instance (thus HTable instance). (I might be missing sth though )

          • Himanshu
          Show
          HBase Review Board added a comment - Message from: "Himanshu Vashishtha" <vashishtha.h@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1639 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java < http://review.cloudera.org/r/876/#comment5505 > Is this list is for scalability. I am not able to visualise a scenario where a Cp has reference to more than one table. Given a region has only one CpHost instance, and though it can a multi CpImpls in it, but each of them will have its own Environment instance (thus HTable instance). (I might be missing sth though ) Himanshu
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-10-25 06:49:15, Himanshu Vashishtha wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 343

          > <http://review.cloudera.org/r/876/diff/7/?file=14190#file14190line343>

          >

          > What is its purpose here? I couldn't see it being used as of now. Is it for some future functionality.

          The access controller coprocessor (HBASE-3025) needs a CatalogTracker. Other future functionality is also considered.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1646
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-10-25 06:49:15, Himanshu Vashishtha wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 343 > < http://review.cloudera.org/r/876/diff/7/?file=14190#file14190line343 > > > What is its purpose here? I couldn't see it being used as of now. Is it for some future functionality. The access controller coprocessor ( HBASE-3025 ) needs a CatalogTracker. Other future functionality is also considered. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1646 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Mingjie Lai" <mjlai09@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/
          -----------------------------------------------------------

          (Updated 2010-11-08 14:41:41.575886)

          Review request for hbase, stack, Andrew Purtell, and Jonathan Gray.

          Changes
          -------

          Changes:

          • Addressed Stack's comments.
          • Renamed CommandTarget to Endpoint (still up to decide whether it's better or not)
          • Improved package-info.
          • Refined some of RegionObserver method signatures, i.e., preGet(), etc.
          • Added hooks for HTable.increment(Increment).

          Summary (updated)
          -------

          The diff actually contains 2 seperate patches: HBase-2001 and the one for (HBASE-2002+HBASE-2321). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002, HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch.

          Eventually the patch here should be committed after 2001 and 2321. I will make another patch after they got checked in.

          Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor:

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java

          ==========================

          (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.)

          Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader.

          Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are:

          • Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations.
          • RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc.
          • Endpoint: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server.

          Coprocessor:
          A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally.

          Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly.

          RegionObserver
          If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region.

          Endpoint:
          Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points.

          Through Endpoint and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can create a new method, specify passed parameters and return types for the method. And the new Endpoint methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...).

          Coprocess loading
          A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table.

          (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading)

          This addresses bug HBase-2001.
          http://issues.apache.org/jira/browse/HBase-2001

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/client/Action.java 556ea81
          src/main/java/org/apache/hadoop/hbase/client/HConnection.java 3235a9b
          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1748a4f
          src/main/java/org/apache/hadoop/hbase/client/HTable.java 69dc916
          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java ade3e21
          src/main/java/org/apache/hadoop/hbase/client/MultiAction.java c6ea838
          src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 1ac7671
          src/main/java/org/apache/hadoop/hbase/client/coprocessor/Batch.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/coprocessor/Exec.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/coprocessor/ExecResult.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/client/coprocessor/package-info.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 30dd877
          src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java f8d8af7
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 4f4828b
          src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java e4c356d
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java cf30cf9
          src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 209c291
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java dd2955d
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ab183e5
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 1309f93
          src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 1bcde8c
          src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java ace7997
          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 5f05079
          src/main/resources/hbase-default.xml 5fafe65
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java PRE-CREATION

          Diff: http://review.cloudera.org/r/876/diff

          Testing
          -------

          Thanks,

          Mingjie

          Show
          HBase Review Board added a comment - Message from: "Mingjie Lai" <mjlai09@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/ ----------------------------------------------------------- (Updated 2010-11-08 14:41:41.575886) Review request for hbase, stack, Andrew Purtell, and Jonathan Gray. Changes ------- Changes: Addressed Stack's comments. Renamed CommandTarget to Endpoint (still up to decide whether it's better or not) Improved package-info. Refined some of RegionObserver method signatures, i.e., preGet(), etc. Added hooks for HTable.increment(Increment). Summary (updated) ------- The diff actually contains 2 seperate patches: HBase-2001 and the one for ( HBASE-2002 + HBASE-2321 ). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002 , HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch. Eventually the patch here should be committed after 2001 and 2321. I will make another patch after they got checked in. Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor: src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java ========================== (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.) Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader. Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are: Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations. RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc. Endpoint: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server. Coprocessor: A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally. Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly. RegionObserver If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region. Endpoint: Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points. Through Endpoint and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can create a new method, specify passed parameters and return types for the method. And the new Endpoint methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...). Coprocess loading A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table. (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading) This addresses bug HBase-2001. http://issues.apache.org/jira/browse/HBase-2001 Diffs (updated) src/main/java/org/apache/hadoop/hbase/client/Action.java 556ea81 src/main/java/org/apache/hadoop/hbase/client/HConnection.java 3235a9b src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1748a4f src/main/java/org/apache/hadoop/hbase/client/HTable.java 69dc916 src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java ade3e21 src/main/java/org/apache/hadoop/hbase/client/MultiAction.java c6ea838 src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 1ac7671 src/main/java/org/apache/hadoop/hbase/client/coprocessor/Batch.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/coprocessor/Exec.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/coprocessor/ExecResult.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/client/coprocessor/package-info.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 30dd877 src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java f8d8af7 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java 4f4828b src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java e4c356d src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java cf30cf9 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/HMaster.java 209c291 src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java dd2955d src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ab183e5 src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 1309f93 src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 1bcde8c src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java ace7997 src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 5f05079 src/main/resources/hbase-default.xml 5fafe65 src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java PRE-CREATION Diff: http://review.cloudera.org/r/876/diff Testing ------- Thanks, Mingjie
          Hide
          Mingjie Lai added a comment -

          Modified package-info.html file.

          Show
          Mingjie Lai added a comment - Modified package-info.html file.
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          On 2010-10-05 23:10:58, stack wrote:

          > src/main/java/org/apache/hadoop/hbase/client/Action.java, line 30

          > <http://review.cloudera.org/r/876/diff/7/?file=14158#file14158line30>

          >

          > I took a look at the package-info.html. Very nice doc. One thought though was that the batch methods do not seem to be instrumented. Are they? The bulk of inserts are done by multiput now.

          >

          > Maybe link to the wiki page when you say this in package-info.html '....implement role-based access control for HBase'

          >

          > Fix this 'These code will be triggerd with existing...'

          >

          > BaseRegionObserver as the name of the class that implements BOTH Coprocessor and RegionObserver with sensible defaults seems off... it'd make sense as the name of an implemenation of RegionObserver but not of both. Is there a better name to give it – even BaseRegionObserverCoprocessor? Unless BaseObserver already implements Coprocessor?

          >

          > Should this also say that methods can be new also? '...i.e., you can specify new passed parameters and return types for a method. '

          >

          > CommandTarget is a strange name for an host of arbitrary user-designed methods. Can we come up w/ something more telling? Notions that come to mind are Substrate, Platform – i.e. stuff you build up on.

          >

          > Minor.. fix '...the actually implemention class running...'

          >

          > Fix this '...How is the client side example of calling...'

          >

          > The example is missing a bit of code that would help along its illustration.... a few comments would help too.... but this is a minor criticism. Not important. I get the gist (Folks interested in CP need to start with this page – it makes grokking the code the easier).

          >

          > This page would seem to indicate CPs can be chained. Am I reading that wrong? (See 'Load from configuration') Over in Gary review, he was saying on CP per region only.

          >

          >

          > Usually attribute names are upper-cased. Here we have 'Coprocessor$1' (that $1is intentional right?)

          >

          > This functionality, if its working, is amazing.

          >

          >

          >

          Mingjie Lai wrote:

          @stack:

          I didn't realize you posted a comment until last week, since your comments here didn't get pushed to jira, neither emails sent to dev@hbase.

          Thanks for your comments. I will address them very soon. But before that I'd like to finalize the name of ``CommandTarget'':

          You said, ``CommandTarget is a strange name for an host of arbitrary user-designed methods. Can we come up w/ something more telling? Notions that come to mind are Substrate, Platform – i.e. stuff you build up on.''

          Some of us suggested to use ``Endpoint'' instead of CommandTarget. Do you like it better? (I'm not really good at naming stuff)

          After finalizing the name, I will make the changes to both source code and package-info. And post a patch here.

          Thanks,

          Mingjie

          I'm not good at naming either.... Endpoint seems more 'generic', less loaded than 'CommandTarget'. If you fellas working with this stuff think that a better name then thats good by me.

          • stack

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1438
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: stack@duboce.net On 2010-10-05 23:10:58, stack wrote: > src/main/java/org/apache/hadoop/hbase/client/Action.java, line 30 > < http://review.cloudera.org/r/876/diff/7/?file=14158#file14158line30 > > > I took a look at the package-info.html. Very nice doc. One thought though was that the batch methods do not seem to be instrumented. Are they? The bulk of inserts are done by multiput now. > > Maybe link to the wiki page when you say this in package-info.html '....implement role-based access control for HBase' > > Fix this 'These code will be triggerd with existing...' > > BaseRegionObserver as the name of the class that implements BOTH Coprocessor and RegionObserver with sensible defaults seems off... it'd make sense as the name of an implemenation of RegionObserver but not of both. Is there a better name to give it – even BaseRegionObserverCoprocessor? Unless BaseObserver already implements Coprocessor? > > Should this also say that methods can be new also? '...i.e., you can specify new passed parameters and return types for a method. ' > > CommandTarget is a strange name for an host of arbitrary user-designed methods. Can we come up w/ something more telling? Notions that come to mind are Substrate, Platform – i.e. stuff you build up on. > > Minor.. fix '...the actually implemention class running...' > > Fix this '...How is the client side example of calling...' > > The example is missing a bit of code that would help along its illustration.... a few comments would help too.... but this is a minor criticism. Not important. I get the gist (Folks interested in CP need to start with this page – it makes grokking the code the easier). > > This page would seem to indicate CPs can be chained. Am I reading that wrong? (See 'Load from configuration') Over in Gary review, he was saying on CP per region only. > > > Usually attribute names are upper-cased. Here we have 'Coprocessor$1' (that $1is intentional right?) > > This functionality, if its working, is amazing. > > > Mingjie Lai wrote: @stack: I didn't realize you posted a comment until last week, since your comments here didn't get pushed to jira, neither emails sent to dev@hbase. Thanks for your comments. I will address them very soon. But before that I'd like to finalize the name of ``CommandTarget'': You said, ``CommandTarget is a strange name for an host of arbitrary user-designed methods. Can we come up w/ something more telling? Notions that come to mind are Substrate, Platform – i.e. stuff you build up on.'' Some of us suggested to use ``Endpoint'' instead of CommandTarget. Do you like it better? (I'm not really good at naming stuff) After finalizing the name, I will make the changes to both source code and package-info. And post a patch here. Thanks, Mingjie I'm not good at naming either.... Endpoint seems more 'generic', less loaded than 'CommandTarget'. If you fellas working with this stuff think that a better name then thats good by me. stack ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1438 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: stack@duboce.net

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1930
          -----------------------------------------------------------

          Ship it!

          +1 on commit to TRUNK. I think all below can be cleaned up on commit (Andrew, you going to commit?)

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          <http://review.cloudera.org/r/876/#comment6139>

          Check in here. Looks like tabs? review board reporting it as whitespace.

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          <http://review.cloudera.org/r/876/#comment6140>

          Usually in hbase code base there are spaces around operations; e.g. around '+'.

          src/main/java/org/apache/hadoop/hbase/client/coprocessor/ExecResult.java
          <http://review.cloudera.org/r/876/#comment6142>

          Be careful. In hbase lines are 80 characters long normally. Fix on commit?

          src/main/java/org/apache/hadoop/hbase/client/coprocessor/ExecResult.java
          <http://review.cloudera.org/r/876/#comment6143>

          I think its ok if these lines > 80 characters

          src/main/java/org/apache/hadoop/hbase/client/coprocessor/package-info.java
          <http://review.cloudera.org/r/876/#comment6144>

          Excellent

          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          <http://review.cloudera.org/r/876/#comment6146>

          Lots of white space in here.

          • stack
          Show
          HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1930 ----------------------------------------------------------- Ship it! +1 on commit to TRUNK. I think all below can be cleaned up on commit (Andrew, you going to commit?) src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < http://review.cloudera.org/r/876/#comment6139 > Check in here. Looks like tabs? review board reporting it as whitespace. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < http://review.cloudera.org/r/876/#comment6140 > Usually in hbase code base there are spaces around operations; e.g. around '+'. src/main/java/org/apache/hadoop/hbase/client/coprocessor/ExecResult.java < http://review.cloudera.org/r/876/#comment6142 > Be careful. In hbase lines are 80 characters long normally. Fix on commit? src/main/java/org/apache/hadoop/hbase/client/coprocessor/ExecResult.java < http://review.cloudera.org/r/876/#comment6143 > I think its ok if these lines > 80 characters src/main/java/org/apache/hadoop/hbase/client/coprocessor/package-info.java < http://review.cloudera.org/r/876/#comment6144 > Excellent src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java < http://review.cloudera.org/r/876/#comment6146 > Lots of white space in here. stack
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          On 2010-11-15 16:51:18, stack wrote:

          > +1 on commit to TRUNK. I think all below can be cleaned up on commit (Andrew, you going to commit?)

          Stack, Yes I plan to commit the patches for HBASE-2001/HBASE-2002/HBASE-2321 onto trunk this week. The dynamic RPC and coprocessor framework changes are largely independent and will go in separately to make the change history in the commit log more informative. We will address your comments before doing so.

          • Andrew

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1930
          -----------------------------------------------------------

          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> On 2010-11-15 16:51:18, stack wrote: > +1 on commit to TRUNK. I think all below can be cleaned up on commit (Andrew, you going to commit?) Stack, Yes I plan to commit the patches for HBASE-2001 / HBASE-2002 / HBASE-2321 onto trunk this week. The dynamic RPC and coprocessor framework changes are largely independent and will go in separately to make the change history in the commit log more informative. We will address your comments before doing so. Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1930 -----------------------------------------------------------
          Hide
          HBase Review Board added a comment -

          Message from: "Mingjie Lai" <mjlai09@gmail.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/
          -----------------------------------------------------------

          (Updated 2010-11-19 14:39:18.378849)

          Review request for hbase, stack, Andrew Purtell, and Jonathan Gray.

          Changes
          -------

          Final patch, ready to be checked in:

          • Truncated white space at line end
          • Rebuilt the patch after HBase-2002 checked in.

          Summary
          -------

          The diff actually contains 2 seperate patches: HBase-2001 and the one for (HBASE-2002+HBASE-2321). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002, HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch.

          Eventually the patch here should be committed after 2001 and 2321. I will make another patch after they got checked in.

          Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor:

          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java

          ==========================

          (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.)

          Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader.

          Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are:

          • Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations.
          • RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc.
          • Endpoint: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server.

          Coprocessor:
          A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally.

          Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly.

          RegionObserver
          If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region.

          Endpoint:
          Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points.

          Through Endpoint and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can create a new method, specify passed parameters and return types for the method. And the new Endpoint methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...).

          Coprocess loading
          A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table.

          (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading)

          This addresses bug HBase-2001.
          http://issues.apache.org/jira/browse/HBase-2001

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java d669de1
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 2ef6432
          src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 1309f93
          src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 1bcde8c
          src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java ace7997
          src/main/resources/hbase-default.xml 630c041
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION

          Diff: http://review.cloudera.org/r/876/diff

          Testing
          -------

          Thanks,

          Mingjie

          Show
          HBase Review Board added a comment - Message from: "Mingjie Lai" <mjlai09@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/ ----------------------------------------------------------- (Updated 2010-11-19 14:39:18.378849) Review request for hbase, stack, Andrew Purtell, and Jonathan Gray. Changes ------- Final patch, ready to be checked in: Truncated white space at line end Rebuilt the patch after HBase-2002 checked in. Summary ------- The diff actually contains 2 seperate patches: HBase-2001 and the one for ( HBASE-2002 + HBASE-2321 ). The reason is that HBase-2001's CommandTarget relies on HBASE-2002 + HBASE-2321 which patches are still under review. I have to include Gary's HBASE-2002 , HBASE-2321 with this diff, since reviewboard is so powerful and it disallow my diff to be based on some unchecked in patch. Eventually the patch here should be committed after 2001 and 2321. I will make another patch after they got checked in. Both HBase-2001 and the dynamic RPC stuff are quite big patches. Total number of lines are more than 7k. I turned back and forth, but still don't have a good idea to create the patch in order to reduce the review pain. However right now I'm putting the whole patch for all the 3 issues. Here the list of file which are only related to coprocessor: src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java ========================== (Here is a brief description. Please find much more details at the package-info.java in the diff. I also post the package-info.html to https://issues.apache.org/jira/browse/HBASE-2001 as an attachment.) Coprocessors are code that runs in-process on each region server. Regions contain references to the coprocessor implementation classes associated with them. Coprocessor classes will be loaded either from local jars on the region server's classpath or via the HDFS classloader. Multiple types of coprocessors are provided to provide sufficient flexibility for potential use cases. Right now there are: Coprocessor: provides region lifecycle management hooks, e.g., region open/close/split/flush/compact operations. RegionObserver: provides hook for monitor table operations from client side, such as table get/put/scan/delete, etc. Endpoint: provides on demand triggers for any arbitrary function executed at a region. One use case is column aggregation at region server. Coprocessor: A coprocessor is required to implement Coprocessor interface so that coprocessor framework can manage it internally. Another design goal of this interface is to provide simple features for making coprocessors useful, while exposing no more internal state or control actions of the region server than necessary and not exposing them directly. RegionObserver If the coprocessor implements the RegionObserver interface it can observe and mediate client actions on the region. Endpoint: Coprocessor and RegionObserver provide certain hooks for injecting user code running at each region. These code will be triggerd with existing HTable and HBaseAdmin operations at the certain hook points. Through Endpoint and dynamic RPC protocol, you can define your own interface communicated between client and region server, i.e., you can create a new method, specify passed parameters and return types for the method. And the new Endpoint methods can be triggered by calling client side dynamic RPC functions – HTable.exec(...). Coprocess loading A customized coprocessor can be loaded by two different ways, by configuration, or by HTableDescriptor for a newly created table. (Currently we don't really have an on demand coprocessor loading machanism for opened regions. However it should be easy to create a dedicated CommandTarget for coprocessor loading) This addresses bug HBase-2001. http://issues.apache.org/jira/browse/HBase-2001 Diffs (updated) src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorException.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/coprocessor/package-info.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java d669de1 src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 2ef6432 src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 1309f93 src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 1bcde8c src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java ace7997 src/main/resources/hbase-default.xml 630c041 src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationProtocol.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java PRE-CREATION Diff: http://review.cloudera.org/r/876/diff Testing ------- Thanks, Mingjie
          Hide
          Mingjie Lai added a comment -

          Final patch for check-in.

          Show
          Mingjie Lai added a comment - Final patch for check-in.
          Hide
          HBase Review Board added a comment -

          Message from: "Andrew Purtell" <apurtell@apache.org>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/876/#review1961
          -----------------------------------------------------------

          Ship it!

          Will commit after running unit tests and verifying all pass.

          • Andrew
          Show
          HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1961 ----------------------------------------------------------- Ship it! Will commit after running unit tests and verifying all pass. Andrew
          Hide
          Andrew Purtell added a comment -

          Committed to trunk. All tests pass locally except TestAdmin#testHundredsOfTable which I also see on plain trunk up on our Hudson.

          Show
          Andrew Purtell added a comment - Committed to trunk. All tests pass locally except TestAdmin#testHundredsOfTable which I also see on plain trunk up on our Hudson.

            People

            • Assignee:
              Mingjie Lai
              Reporter:
              Andrew Purtell
            • Votes:
              1 Vote for this issue
              Watchers:
              21 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development