brfrn169 commented on a change in pull request #2630:
URL: https://github.com/apache/hbase/pull/2630#discussion_r528213068
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java
##########
@@ -60,7 +53,7 @@
*/
@InterfaceAudience.Public
@InterfaceStability.Evolving
-public final class CheckAndMutate extends Mutation {
+public final class CheckAndMutate implements Row {
Review comment:
We haven't released the CheckAndMutate class yet. It's introduced in
2.4.0. We need to commit this change before releasing 2.4.0.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Table.java
##########
@@ -459,9 +459,10 @@ default CheckAndMutateResult checkAndMutate(CheckAndMutate
checkAndMutate) throw
* {@link Put} and {@link Delete} are supported.
*
* @param rm object that specifies the set of mutations to perform atomically
+ * @return results of Increment/Append operations
* @throws IOException
Review comment:
Sure. I will do that.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3657,9 +3657,10 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
return builder.build((Put) m);
} else if (m instanceof Delete) {
return builder.build((Delete) m);
+ } else if (m instanceof Increment) {
+ return builder.build((Increment) m);
} else {
- throw new DoNotRetryIOException("Unsupported mutate type: " +
mutations.get(0)
- .getClass().getSimpleName().toUpperCase());
+ return builder.build((Append) m);
Review comment:
Sure. I will add testing for Append and keep the last else branch
throwing DoNotRetryIOException.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##########
@@ -620,8 +622,22 @@ private CheckAndMutateResult checkAndMutate(HRegion
region, List<ClientProtos.Ac
spaceQuotaEnforcement.getPolicyEnforcement(region).check(del);
mutations.add(del);
break;
+ case INCREMENT:
+ Increment increment =
ProtobufUtil.toIncrement(action.getMutation(), cellScanner);
+ ++countOfCompleteMutation;
+ checkCellSizeLimit(region, increment);
+
spaceQuotaEnforcement.getPolicyEnforcement(region).check(increment);
+ mutations.add(increment);
+ break;
+ case APPEND:
+ Append append = ProtobufUtil.toAppend(action.getMutation(),
cellScanner);
+ ++countOfCompleteMutation;
+ checkCellSizeLimit(region, append);
+ spaceQuotaEnforcement.getPolicyEnforcement(region).check(append);
+ mutations.add(append);
+ break;
default:
- throw new DoNotRetryIOException("Atomic put and/or delete only,
not " + type.name());
+ throw new AssertionError("invalid mutation type : " + type);
Review comment:
Sure. I will revert this change.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ResponseConverter.java
##########
@@ -158,52 +156,30 @@ public static SingleResponse getResult(final
ClientProtos.MutateRequest request,
// If there is an exception from the server, the exception is set at
// the RegionActionResult level, which has been handled above.
if (actions.hasCondition()) {
- Result result = null;
- if (actionResult.getResultOrExceptionCount() > 0) {
- ResultOrException roe = actionResult.getResultOrException(0);
- if (roe.hasResult()) {
- Result r = ProtobufUtil.toResult(roe.getResult(), cells);
- if (!r.isEmpty()) {
- result = r;
- }
- }
- }
- responseValue = new
CheckAndMutateResult(actionResult.getProcessed(), result);
+ results.add(regionName, index, getCheckAndMutateResult(actionResult,
cells));
} else {
- responseValue = actionResult.getProcessed() ?
- ProtobufUtil.EMPTY_RESULT_EXISTS_TRUE :
- ProtobufUtil.EMPTY_RESULT_EXISTS_FALSE;
+ results.add(regionName, index, getMutateRowResult(actionResult,
cells));
}
- results.add(regionName, index, responseValue);
continue;
}
if (actions.hasCondition()) {
- Result result = null;
- if (actionResult.getResultOrExceptionCount() > 0) {
- ResultOrException roe = actionResult.getResultOrException(0);
- Result r = ProtobufUtil.toResult(roe.getResult(), cells);
- if (!r.isEmpty()) {
- result = r;
- }
- }
- responseValue = new CheckAndMutateResult(actionResult.getProcessed(),
result);
- results.add(regionName, 0, responseValue);
+ results.add(regionName, 0, getCheckAndMutateResult(actionResult,
cells));
} else {
+ if (actionResult.hasProcessed()) {
Review comment:
The case where actions.hasCondition() is false and
actionResult.hasProcessed() is true is the case for a result of mutateRow().
After this change, mutateRow() can return results of Increment/Append
operations. That's why we need to add this logic here.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##########
@@ -972,11 +1002,44 @@ private void doBatchOp(final RegionActionResult.Builder
builder, final HRegion r
OperationStatus[] codes = region.batchMutate(mArray, atomic,
HConstants.NO_NONCE,
HConstants.NO_NONCE);
+ if (atomic) {
+ LinkedList<ResultOrException> resultOrExceptions = new LinkedList<>();
Review comment:
The reason why I use LinkedList here is that I need to add a element at
the beginning of the list as follows:
https://github.com/apache/hbase/pull/2630/files#diff-e4052cd5a1f1c93375e3fbc931dc4df220deebc78c0d53fd2435b47fd04cd807R1019-R1020
https://github.com/apache/hbase/pull/2630/files#diff-e4052cd5a1f1c93375e3fbc931dc4df220deebc78c0d53fd2435b47fd04cd807R1031
Still ArrayList is faster in that case?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
