> On 2010-10-03 02:28:20, Lars Francke wrote: > > Sorry for all the whitespace comments :) > > There are a bunch more in the test classes.
Since some of this covers the included patch from HBASE-2321/HBASE-2002, I'll reply to those issues. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/client/Action.java, line 91 > > <http://review.cloudera.org/r/876/diff/5/?file=13544#file13544line91> > > > > The ternary operator does not need braces. Sure, I just find the parens add clarity. I can remove. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line > > 1142 > > <http://review.cloudera.org/r/876/diff/5/?file=13549#file13549line1142> > > > > Should be of Type List<R> not ArrayList<R> Seems less important to me for a local variable, but I'll change. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line > > 1143 > > <http://review.cloudera.org/r/876/diff/5/?file=13549#file13549line1143> > > > > Why is this necessary? You already set the size by using the correct > > constructor. The constructor argument sets initial capacity, not size. In this case we need to set size, which means prefilling the list, since null entries are how we indicate failure for individual rows. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java, line 353 > > <http://review.cloudera.org/r/876/diff/5/?file=13551#file13551line353> > > > > Remove the "public", interfaces don't need that. Sure, copy-n-paste error, will remove. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java, line 398 > > <http://review.cloudera.org/r/876/diff/5/?file=13551#file13551line398> > > > > Remove the "public", interfaces don't need that. Will remove. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java, line 36 > > <http://review.cloudera.org/r/876/diff/5/?file=13565#file13565line36> > > > > public Log? Odd, it's never even used. Removing. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java, line 348 > > <http://review.cloudera.org/r/876/diff/5/?file=13578#file13578line348> > > > > Wrong formatting Should not be included in this patch. Will correct. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java, line 807 > > <http://review.cloudera.org/r/876/diff/5/?file=13580#file13580line807> > > > > Whitespace stuff Should not be included in this patch. Will correct. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java, > > line 62 > > <http://review.cloudera.org/r/876/diff/5/?file=13592#file13592line62> > > > > Whitespace stuff Should not be included in this patch. Will correct. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java, > > line 115 > > <http://review.cloudera.org/r/876/diff/5/?file=13592#file13592line115> > > > > Whitespace stuff Should not be included in this patch. Will correct. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java, > > line 128 > > <http://review.cloudera.org/r/876/diff/5/?file=13593#file13593line128> > > > > Whitespace stuff Should not be included in this patch. Will correct. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java, line 39 > > <http://review.cloudera.org/r/876/diff/5/?file=13567#file13567line39> > > > > private static final Will fix. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java, line 92 > > <http://review.cloudera.org/r/876/diff/5/?file=13570#file13570line92> > > > > Whitespace stuff Should not be included in this patch. Fixing. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java, line 372 > > <http://review.cloudera.org/r/876/diff/5/?file=13551#file13551line372> > > > > 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. Will remove public. Not sure I understand the Map keyed by byte[] comment. That's part of the contract for clients. So all implementors must implement it correctly or indicate they don't support it through something like an UnsupportedOperationException. I suppose I could narrow the type to SortedMap, since that indicates use of a comparator for byte[] keys. What were you suggesting? > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/HServerInfo.java, line 99 > > <http://review.cloudera.org/r/876/diff/5/?file=13543#file13543line99> > > > > The ternary operator does not need braces. Should not actually be included in this patch. Fixing. > On 2010-10-03 02:28:20, Lars Francke wrote: > > src/main/java/org/apache/hadoop/hbase/ipc/Status.java, line 27 > > <http://review.cloudera.org/r/876/diff/5/?file=13581#file13581line27> > > > > Whitespace stuff Should not be included in this patch. Fixing. - Gary ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/876/#review1380 ----------------------------------------------------------- On 2010-10-02 23:53:05, Mingjie Lai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/876/ > ----------------------------------------------------------- > > (Updated 2010-10-02 23:53:05) > > > Review request for hbase, stack, Andrew Purtell, and Jonathan Gray. > > > 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/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/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java > src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.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 > ----- > > 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 > 0feb17c > src/main/java/org/apache/hadoop/hbase/client/HTable.java 0dbf263 > src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 74593bf > 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 > 89f499a > > 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/rest/client/RemoteHTable.java d4166cf > 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 > >
