New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-25242 Add Increment/Append support to RowMutations #2630
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Can you please review this? @Apache9 |
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
OperationStatus[] operationStatuses = batchMutate(m.toArray(new Mutation[0]), true, | ||
HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
|
||
List<Result> results = Arrays.stream(operationStatuses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for JDK8, stream is slow so let's avoid stream on critical read/write path as much as possible for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will change it not to use stream. Thanks.
default: | ||
throw new DoNotRetryIOException("Atomic put and/or delete only, not " + type.name()); | ||
throw new AssertionError("invalid mutation type : " + type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change to AssertionError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MutationType can be set to only Put/Delete/Increment/Append. And this switch statement has all the cases (Put/Delete/Increment/Append) after this change, so it is impossible to go the default case. That's why I change it to AssertionError, which means if we hit this AssertionError, it's something a bug. Do you think we should still use DoNotRetryIOException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you said is also reasonable, but since this is an rpc method, we can not make sure whether the remote side has the same code with us at server side, so I still prefer a DoNotRetryIOException here. Let's not change it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will revert this change.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Apache9 Thank you very much for reviewing this!
Just left some comments. Can you please check them?
I will change this not to use stream in the meanwhile.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
OperationStatus[] operationStatuses = batchMutate(m.toArray(new Mutation[0]), true, | ||
HConstants.NO_NONCE, HConstants.NO_NONCE); | ||
|
||
List<Result> results = Arrays.stream(operationStatuses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will change it not to use stream. Thanks.
default: | ||
throw new DoNotRetryIOException("Atomic put and/or delete only, not " + type.name()); | ||
throw new AssertionError("invalid mutation type : " + type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MutationType can be set to only Put/Delete/Increment/Append. And this switch statement has all the cases (Put/Delete/Increment/Append) after this change, so it is impossible to go the default case. That's why I change it to AssertionError, which means if we hit this AssertionError, it's something a bug. Do you think we should still use DoNotRetryIOException?
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
Show resolved
Hide resolved
d086fea
to
399e3fd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ping @Apache9 . Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OVerall LGTM. Just a small question and some minor nits.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: could remove the empty throws so we could fix on checkstyle warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will do that.
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ResponseConverter.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
Show resolved
Hide resolved
@@ -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<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use ArrayList instead of LinkedList. ArrayList is faster than LinkedList for most cases. I can not recall the exact number but at least for thousands elements ArrayList is still faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if we want to add element at first then ArrayList is not suitable. But usually we could use other array based data structure such as ArrayDeque. Let me take a look at the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java
Show resolved
Hide resolved
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will do that.
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
default: | ||
throw new DoNotRetryIOException("Atomic put and/or delete only, not " + type.name()); | ||
throw new AssertionError("invalid mutation type : " + type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will revert this change.
@@ -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<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ResponseConverter.java
Show resolved
Hide resolved
399e3fd
to
2a3bf95
Compare
I just changed the patch regarding the following:
Can you please take a look at this? @Apache9 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2a3bf95
to
2baf627
Compare
I made a mistake and the compile in the last QA failed.. Just fixed it and pushed the latest patch. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
} | ||
|
||
if (results.isEmpty()) { | ||
resultOrExceptions.add(0, getResultOrException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we could just call builder.addResultOrException directly to add the first ResultOrException? And then call addAllResultOrException to add the remaining ResultOrExceptions in the list, so we do not need to add a new element in front?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I will do that. Thanks.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
Show resolved
Hide resolved
2baf627
to
c58d778
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Please wait for @Apache9 's approval before merging.
c58d778
to
66a915d
Compare
I just added the following comment: Can you please take a look at this? @Apache9 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks for your patient.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Are the failed UTs related? TestAsyncTable? |
Seems the failed UTs for TestAsyncTable are not related to this patch. But it looks like the root cause of the failed UTs are that we don't wait until AsyncTable.put() completes at the following lines: hbase/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTable.java Line 998 in d81b541
hbase/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTable.java Line 1055 in d81b541
We need to call get() method to wait until AsyncTable.put() completes as follows:
I will fix it and re-push it. |
66a915d
to
b76370a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I don't think the failed UTs in the last QA are related to this patch, but I will trigger QA again just in case. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
I don't think the failed UTs are related to this change. The failed UTs were successful locally. I will merge this PR. |
No description provided.