You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Benoit Sigoure <ts...@gmail.com> on 2010/08/15 05:04:12 UTC
Re: Review Request: HBASE-1845 MultiGet, MultiDelete,
and MultiPut - batched to the appropriate region servers
-----------------------------------------------------------
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
>
>