hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benoit Sigoure" <tsuna...@gmail.com>
Subject Re: Review Request: HBASE-1845 MultiGet, MultiDelete, and MultiPut - batched to the appropriate region servers
Date Sun, 15 Aug 2010 03:04:12 GMT

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


-1 overall.

The good thing about this change is that it addresses a ~third of HBASE-2898.
The very bad thing about this change is that it generalizes the disease that started with
MultiPut, which is described in HBASE-2898.  I *really* don't want HBASE-2898 to spread like
cancer.  I believe multi-everything is the way to go, but we need to change more things to
also cure HBASE-2898 instead of spreading it.

The MultiResponse does the right thing in the sense that it gives us the result of each individual
Action, instead of what the old MultiPutResponse does which is to give us the "index" of the
first failed Put.  This is necessary but not sufficient.  When there's a failure for a particular
Action, the client *needs* to know what the failure was for this particular Action.  If it's
a NoSuchColumnFamilyException, then the client should bail out immediately and escalate the
exception to the user.  Other exceptions (like NotServingRegionException) can be handled by
the client.  Maybe instead of storing a Result per Action, we could store an Object with is
either a Result or an Exception.


http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Action.java
<http://review.cloudera.org/r/151/#comment2944>

    Missing copyright header.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Action.java
<http://review.cloudera.org/r/151/#comment2946>

    Missing javadoc.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Action.java
<http://review.cloudera.org/r/151/#comment2945>

    Why is the `Row' called `action'?
    edit: Ah OK I get it, `Row' is a very poorly named interface.  Ignore my comment.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Action.java
<http://review.cloudera.org/r/151/#comment2943>

    Kill ALL trailing whitespaces in all the files.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Action.java
<http://review.cloudera.org/r/151/#comment2973>

    I don't like the fact that you have to store the original order of the `Action' instances.
 I understand you need to do this because the server sorts the actions, but I'd be in favor
of doing the sorting on the client side and removing this unnecessary complication.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Get.java
<http://review.cloudera.org/r/151/#comment2947>

    why is the argument named `del'?



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
<http://review.cloudera.org/r/151/#comment2971>

    Please move the @return after all the @param.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
<http://review.cloudera.org/r/151/#comment2948>

    Please use the same style as in the rest of the file.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
<http://review.cloudera.org/r/151/#comment2969>

    I don't see the point in doing that.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
<http://review.cloudera.org/r/151/#comment2970>

    No spaces before the `[]'.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2949>

    No spaces inside parentheses.  Stick to the existing coding style.
    It doesn't seem necessary to cast `list' to a `List'.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2950>

    This is not an IOException, it's an IllegalArgumentException.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2951>

    Instead of doing that, give `list' in argument to the constructor of ArrayList, so that
the list can be sized and constructed properly immediately.  This avoids an unnecessary array
resize.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2952>

    Unnecessary parentheses on the RHS.
    Also this variable can be declared final.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2953>

    This is bad.  Don't do that.
    Please read http://www.ibm.com/developerworks/java/library/j-jtp05236.html



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2954>

    Missing spaces after the commas.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2955>

    Missing space after the comma.  Please fix all of those.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2956>

    Above you used just `Entry', here you use `Map.Entry'.  Stick to one or the other.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2957>

    Missing spaces around the equal sign.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2958>

    This is also improperly handling InterruptedException.  I realize the code was already
wrong but since you're changing it, you may as well fix it.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2972>

    Remove the unnecessary cast to `DoNotRetryIOException'.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2959>

    Missing spaces around `=', `<' and after the last `;'.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.cloudera.org/r/151/#comment2960>

    Remove the extra spaces inside the parentheses and the unnecessary cast to a plain `List'.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.cloudera.org/r/151/#comment2961>

    Thanks for writing that javadoc.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiAction.java
<http://review.cloudera.org/r/151/#comment2962>

    Technically, users won't use this class, so it doesn't need to be made `public'.  Also,
it could be marked as `final'.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiAction.java
<http://review.cloudera.org/r/151/#comment2963>

    Bad English: "with the attempting to locate"



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiPut.java
<http://review.cloudera.org/r/151/#comment2964>

    When adding a deprecation warning, is it possible in Java to specify an arbitrary message
to explain what should be used instead?  If not, then please at least mention that in the
javadoc.
    This also holds true for MultiPutResponse below.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/151/#comment2965>

    Poorly named variable.  It's not a row, it's an action...



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/151/#comment2966>

    The fact that you have to introspect the type in order to know what to do makes me feel
uneasy.  It shows that you're using the wrong interface for what you're trying to achieve.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/151/#comment2967>

    Don't throw an exception without a message describing why the exception was thrown.



http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/151/#comment2968>

    Remove this unnecessary line you added.


- Benoit


On 2010-06-06 23:23:22, Marc Limotte wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/151/
> -----------------------------------------------------------
> 
> (Updated 2010-06-06 23:23:22)
> 
> 
> Review request for hbase.
> 
> 
> Summary
> -------
> 
> Updated the patch to work on the trunk.  New unit tests are in TestMultiParallel.java,
which replaces TestMultiParallelPut.java.
> 
> 
> This addresses bug HBASE-1845.
>     http://issues.apache.org/jira/browse/HBASE-1845
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Action.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Get.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiAction.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiPut.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiPutResponse.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiResponse.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Row.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
951973 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestMultiParallel.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/TestMultiParallelPut.java
951973 
> 
> Diff: http://review.cloudera.org/r/151/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marc
> 
>


Mime
View raw message