----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/816/#review1214 -----------------------------------------------------------
I did about half. This is kinda amazing Gary. Its going to be crazy. I can't comment on implementation yet because don't grok it yet. Can you help me figure how it'll be used? (Keep on w/ the simple ping/hello implemenation). How would an application make use of this. What kinda exceptions is application going to get? src/main/java/org/apache/hadoop/hbase/client/Action.java <http://review.cloudera.org/r/816/#comment4135> On your overarching comment: + One patch is fine. + Should the key be the encoded region name rather than regionname for map of regions to protocols since the latter can be fat? I think I need some more illustration of how the calls will be made. Seems super awkward at moment. Keep using the ping coprocessor as example. Its basic. Basic is good for me. What you thinking for the batch calls? When you say "but this does not mesh well with the current client-side interface."... whats this mean? You want to change the client? src/main/java/org/apache/hadoop/hbase/client/Action.java <http://review.cloudera.org/r/816/#comment4127> Good src/main/java/org/apache/hadoop/hbase/client/ExecResult.java <http://review.cloudera.org/r/816/#comment4136> What will a value be? Anythign? src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java <http://review.cloudera.org/r/816/#comment4137> Good src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java <http://review.cloudera.org/r/816/#comment4138> Has white space on end of line You want to catch these IEs? We are moving toward letting them out or at least remarking the Thread w/ the interrupted flag (thread.interrupt). src/main/java/org/apache/hadoop/hbase/client/HTable.java <http://review.cloudera.org/r/816/#comment4139> What if the remote region does not have the configured protocol? Can that happen? How does remote region get freighted w/ the coprocessor? - stack On 2010-09-10 17:29:57, Gary Helmling wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/816/ > ----------------------------------------------------------- > > (Updated 2010-09-10 17:29:57) > > > Review request for hbase, Andrew Purtell and Jonathan Gray. > > > Summary > ------- > > This is really two separate patches in one, though with some overlapping > changes. If necessary I can split them apart for separate review. Please > let me know if that would make review easier. > > Part 1: > ============== > Port over of HADOOP-6422 to the HBase RPC code. The goal of this change is > to allow alternate RPC client/server implementations to be enabled through a > simple configuration change. Ultimately I would like to use this to allow > secure RPC to be enabled through configuration, while not blocking normal > (current) RPC operation on non-secure Hadoop versions. > > This portion of the patch abstracts out two interfaces from the RPC code: > > RpcEngine: HBaseRPC uses this to obtain proxy instances for client calls and > server instances for HMaster and HRegionServer > RpcServer: this allows differing RPC server implementations, breaking the > dependency on HBaseServer > > The bulk of the current code from HBaseRPC is moved into WritableRpcEngine > and is unchanged other than the interface requirements. So the current call > path remains the same, other than the HBaseRPC.getProtocolEngine() > abstraction. > > > Part 2: > =============== > The remaining changes provide server-side hooks for registering new RPC > protocols/handlers (per-region to support coprocessors), and client side > hooks to support dynamic execution of the registered protocols. > > The new RPC protocol actions are constrained to > org.apache.hadoop.hbase.ipc.CoprocessorProtocol implementations (which > extends VersionedProtocol) to prevent arbitrary execution of methods against > HMasterInterface, HRegionInterface, etc. > > For protocol handler registration, HRegionServer provides a new method: > > public <T extends CoprocessorProtocol> boolean registerProtocol( > byte[] region, Class<T> protocol, T handler) > > which builds a Map of region name to protocol instances for dispatching > client calls. > > > Client invocations are performed through HTable, which adds the following > methods: > > > public <T extends CoprocessorProtocol> T proxy(Class<T> protocol, Row row) > > This directly returns a proxy instance to the CoprocessorProtocol > implementation registered for the region serving row "row". Any method calls > will be proxied to the region's server and invoked using the map of > registered region name -> handler instances. > > Calls directed against multiple rows are a bit more complicated. They are > supported with the methods: > > public <T extends CoprocessorProtocol, R> void exec( > Class<T> protocol, List<? extends Row> rows, > BatchCall<T,R> callable, BatchCallback<R> callback) > > public <T extends CoprocessorProtocol, R> void exec( > Class<T> protocol, RowRange range, > BatchCall<T,R> callable, BatchCallback<R> callback) > > where BatchCall and BatchCallback are simple interfaces defining the methods > to be called and a callback instance to be invoked for each result. > > For the sample CoprocessorProtocol interface: > > interface PingProtocol extends CoprocessorProtocol { > public String ping(); > public String hello(String name); > } > > a client invocation might look like: > > final Map<byte[],R> results = new TreeMap<byte[],R>(...) > List<Row> rows = ... > table.exec(PingProtocol.class, rows, > new HTable.BatchCall<PingProtocol,String>() { > public String call(PingProtocol instance) { > return instance.ping(); > } > }, > new BatchCallback<R>(){ > public void update(byte[] region, byte[] row, R value) { > results.put(region, value); > } > }); > > The BatchCall.call() method will be invoked for each row in the passed in > list, and the BatchCallback.update() method will be invoked for each return > value. However, currently the PingProtocol.ping() invocation will result in > a separate RPC call per row, which is less that ideal. > > Support is in place to make use of the HRegionServer.multi() invocations for > batched RPC (see the org.apache.hadoop.hbase.client.Exec class), but this > does not mesh well with the current client-side interface. > > In addition to standard code review, I'd appreciate any thoughts on the > client interactions in particular, and whether they would meet some of the > anticipated uses of coprocessors. > > > This addresses bugs HBASE-2002 and HBASE-2321. > http://issues.apache.org/jira/browse/HBASE-2002 > http://issues.apache.org/jira/browse/HBASE-2321 > > > 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 > f1fa836 > src/main/java/org/apache/hadoop/hbase/client/HTable.java 0dbf263 > src/main/java/org/apache/hadoop/hbase/client/MultiAction.java dc0e203 > src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java 60008b1 > 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/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 134288b > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java d3c6c21 > src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java c3e24d9 > 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 adc6292 > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 1e046f9 > src/main/resources/hbase-default.xml 2fcfcc8 > > src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java > PRE-CREATION > > Diff: http://review.cloudera.org/r/816/diff > > > Testing > ------- > > > Thanks, > > Gary > >
