Return-Path: Delivered-To: apmail-hbase-dev-archive@www.apache.org Received: (qmail 10411 invoked from network); 15 Sep 2010 04:08:41 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 15 Sep 2010 04:08:41 -0000 Received: (qmail 45532 invoked by uid 500); 15 Sep 2010 04:08:40 -0000 Delivered-To: apmail-hbase-dev-archive@hbase.apache.org Received: (qmail 45325 invoked by uid 500); 15 Sep 2010 04:08:38 -0000 Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hbase.apache.org Delivered-To: mailing list dev@hbase.apache.org Received: (qmail 45316 invoked by uid 99); 15 Sep 2010 04:08:37 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 15 Sep 2010 04:08:37 +0000 X-ASF-Spam-Status: No, hits=1.8 required=10.0 tests=FH_HELO_EQ_D_D_D_D,MIME_QP_LONG_LINE,SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (athena.apache.org: 184.73.217.71 is neither permitted nor denied by domain of stack@duboce.net) Received: from [184.73.217.71] (HELO ip-10-202-7-187.ec2.internal) (184.73.217.71) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 15 Sep 2010 04:08:32 +0000 Received: from ip-10-202-7-187.ec2.internal (localhost [127.0.0.1]) by ip-10-202-7-187.ec2.internal (Postfix) with ESMTP id D0FDA8A1FA; Wed, 15 Sep 2010 04:08:10 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: Review Request: [HBASE-2321] [HBASE-2002] Add support for per-region dynamically registered RPC endpoints for coprocessors and allow configurable RPC client/server implementations From: stack@duboce.net To: "Andrew Purtell" , "Jonathan Gray" Date: Wed, 15 Sep 2010 04:08:10 -0000 Message-ID: <20100915040810.20451.52506@ip-10-202-7-187.ec2.internal> Cc: "Gary Helmling" , jiraposter@review.hbase.org, dev@hbase.apache.org, stack@duboce.net In-Reply-To: <20100911002957.2013.76254@ip-10-202-7-187.ec2.internal> References: <20100911002957.2013.76254@ip-10-202-7-187.ec2.internal> ----------------------------------------------------------- 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 c= an'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 implemenat= ion). 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 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. S= eems 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 Good src/main/java/org/apache/hadoop/hbase/client/ExecResult.java What will a value be? Anythign? src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java Good src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 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 What if the remote region does not have the configured protocol? Can t= hat 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: > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 throu= gh a simple configuration change. Ultimately I would like to use this to a= llow secure RPC to be enabled through configuration, while not blocking nor= mal (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 WritableRpcEngin= e and is unchanged other than the interface requirements. So the current c= all path remains the same, other than the HBaseRPC.getProtocolEngine() abst= raction. > = > = > Part 2: > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > The remaining changes provide server-side hooks for registering new RPC p= rotocols/handlers (per-region to support coprocessors), and client side hoo= ks to support dynamic execution of the registered protocols. > = > The new RPC protocol actions are constrained to org.apache.hadoop.hbase.i= pc.CoprocessorProtocol implementations (which extends VersionedProtocol) to= prevent arbitrary execution of methods against HMasterInterface, HRegionIn= terface, etc. > = > For protocol handler registration, HRegionServer provides a new method: > = > public boolean registerProtocol( > byte[] region, Class protocol, T handler) > = > which builds a Map of region name to protocol instances for dispatching c= lient calls. > = > = > Client invocations are performed through HTable, which adds the following= methods: > = > = > public T proxy(Class protocol, Row r= ow) > = > This directly returns a proxy instance to the CoprocessorProtocol impleme= ntation 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 ar= e supported with the methods: > = > public void exec( > Class protocol, List rows, > BatchCall callable, BatchCallback callback) > = > public void exec( > Class protocol, RowRange range, > BatchCall callable, BatchCallback callback) > = > where BatchCall and BatchCallback are simple interfaces defining the meth= ods 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 results =3D new TreeMap(...) > List rows =3D ... > table.exec(PingProtocol.class, rows, > new HTable.BatchCall() { > public String call(PingProtocol instance) { > return instance.ping(); > } > }, > new BatchCallback(){ > 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 retur= n 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 th= is does not mesh well with the current client-side interface. > = > In addition to standard code review, I'd appreciate any thoughts on the c= lient interactions in particular, and whether they would meet some of the a= nticipated 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-CREATI= ON = > src/main/java/org/apache/hadoop/hbase/client/HConnection.java 65f7618 = > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java f1= fa836 = > 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 83f62= 3d = > src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CRE= ATION = > src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-= CREATION = > src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREAT= ION = > 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-CR= EATION = > src/main/java/org/apache/hadoop/hbase/master/HMaster.java adc6292 = > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1= e046f9 = > src/main/resources/hbase-default.xml 2fcfcc8 = > src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProt= ocol.java PRE-CREATION = > = > Diff: http://review.cloudera.org/r/816/diff > = > = > Testing > ------- > = > = > Thanks, > = > Gary > = >