Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Received-SPF: neutral (nike.apache.org: 184.73.217.71 is neither permitted nor
 denied by domain of ghelmling@gmail.com)
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: "Gary Helmling" <ghelmling@gmail.com>
To: "Andrew Purtell" <apurtell@apache.org>, "Jonathan Gray" <jgray@apache.org>
Date: Fri, 17 Sep 2010 22:20:58 -0000
Message-ID: <20100917222058.20451.56096@ip-10-202-7-187.ec2.internal>
Cc: "Gary Helmling" <ghelmling@gmail.com>, 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/
-----------------------------------------------------------

(Updated 2010-09-17 15:20:58.111896)


Review request for hbase, Andrew Purtell and Jonathan Gray.


Changes
-------

Merged in latest changes from trunk (including HBASE-2782 and HBASE-2941), =
and addressed some review comments.

Changes since last diff:

- pulled up HBaseRPC.Invocation (WritableRpcEngine.Invocation in previous d=
iff) to top level class
- moved registerProtocol() and exec() methods from HRegionServer down into =
HRegion.  This should allow us to avoid handling region transitions for reg=
istered handlers up in HRS and get us cleaner tie-ins to coprocessors
- renamed HRS.regionExec() -> HRS.exec()
- removed swallowing of IOException in ExecRPCInvoker
- added explicit HBaseRPC.UnknownProtocolException as a DoNotRetryIOExcepti=
on subclass
- propagate ExecutionException and InterruptedException in HConnectionManag=
er.TableServers.processExecs() back up to client
- added some missing javadoc

I'll update with some docbook documentation and some expanded examples as s=
oon as ready, but wanted to get the code changes up for now.


Summary
-------

This is really two separate patches in one, though with some overlapping ch=
anges.  If necessary I can split them apart for separate review.  Please le=
t 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 through=
 a simple configuration change.  Ultimately I would like to use this to all=
ow secure RPC to be enabled through configuration, while not blocking norma=
l (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 an=
d server instances for HMaster and HRegionServer
RpcServer: this allows differing RPC server implementations, breaking the d=
ependency 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 cal=
l path remains the same, other than the HBaseRPC.getProtocolEngine() abstra=
ction.


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 pro=
tocols/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 p=
revent arbitrary execution of methods against HMasterInterface, HRegionInte=
rface, 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 cli=
ent calls.


Client invocations are performed through HTable, which adds the following m=
ethods:


  public <T extends CoprocessorProtocol> T proxy(Class<T> protocol, Row row)

This directly returns a proxy instance to the CoprocessorProtocol implement=
ation registered for the region serving row "row".  Any method calls will b=
e proxied to the region's server and invoked using the map of registered re=
gion 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 method=
s 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 =3D new TreeMap<byte[],R>(...)
    List<Row> rows =3D ...
    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 l=
ist, and the BatchCallback.update() method will be invoked for each return =
value.  However, currently the PingProtocol.ping() invocation will result i=
n a separate RPC call per row, which is less that ideal.

Support is in place to make use of the HRegionServer.multi() invocations fo=
r 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 cli=
ent interactions in particular, and whether they would meet some of the ant=
icipated 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 (updated)
-----

  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 be14=
546 =

  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/io/HbaseObjectWritable.java 83f623d =

  src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java PRE-CREAT=
ION =

  src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorProtocol.java PRE-CR=
EATION =

  src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java PRE-CREATIO=
N =

  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 ae3dba9 =

  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-CREA=
TION =

  src/main/java/org/apache/hadoop/hbase/master/HMaster.java c1b80eb =

  src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java fdef130 =

  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1ec=
7f4e =

  src/main/resources/hbase-default.xml 5452fd1 =

  src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtoc=
ol.java PRE-CREATION =


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


Testing
-------


Thanks,

Gary