[
https://issues.apache.org/jira/browse/HBASE-1845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12898659#action_12898659
]
HBase Review Board commented on HBASE-1845:
-------------------------------------------
Message from: "Benoit Sigoure" <[email protected]>
-----------------------------------------------------------
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
> MultiGet, MultiDelete, and MultiPut - batched to the appropriate region
> servers
> -------------------------------------------------------------------------------
>
> Key: HBASE-1845
> URL: https://issues.apache.org/jira/browse/HBASE-1845
> Project: HBase
> Issue Type: New Feature
> Reporter: Erik Holstad
> Fix For: 0.90.0
>
> Attachments: batch.patch, hbase-1845_0.20.3.patch,
> hbase-1845_0.20.5.patch, multi-v1.patch
>
>
> I've started to create a general interface for doing these batch/multi calls
> and would like to get some input and thoughts about how we should handle this
> and what the protocol should
> look like.
> First naive patch, coming soon.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
