HBASE-24650 Change the return types of the new checkAndMutate methods… #1991
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Apache9
requested changes
Jun 28, 2020
| * Represents a result of a CheckAndMutate operation | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving |
There was a problem hiding this comment.
For IA.Public, we do not use the IS annotation since it should always be Stable.
There was a problem hiding this comment.
Okay. I will remove the IS annotation. Thanks.
| @@ -349,7 +349,7 @@ private void preCheck() { | |||
| loc, stub, mutation, | |||
| (rn, rm) -> RequestConverter.buildMutateRequest(rn, row, family, qualifier, op, value, | |||
| null, timeRange, rm), | |||
| resp -> resp.getExists())) | |||
| resp -> ((CheckAndMutateResult) resp).isSuccess())) | |||
There was a problem hiding this comment.
We have already make use of generic type so why here we still need to cast?
There was a problem hiding this comment.
I will modify it to make use of generic type. Thanks.
| } | ||
|
|
||
| // We need the MultiRequest when constructing the org.apache.hadoop.hbase.client.MultiResponse, | ||
| // so here I write a new method as I do not want to change the abstraction of call method. | ||
| private <RESP> CompletableFuture<RESP> mutateRow(HBaseRpcController controller, | ||
| HRegionLocation loc, ClientService.Interface stub, RowMutations mutation, | ||
| Converter<MultiRequest, byte[], RowMutations> reqConvert, | ||
| Function<Result, RESP> respConverter) { | ||
| Function<Object, RESP> respConverter) { |
There was a problem hiding this comment.
Object is too generic. Please explain why we need to do this change?
There was a problem hiding this comment.
The reason why I changed Result to Object here is that this method is used by both checkAndMutate() and mutateRow() and we need to use different types each case (respectively CheckAndMutateResult and Result). However as you mentioned, we can make use of generic type here. I will modify it. Thanks.
This comment has been minimized.
This comment has been minimized.
brfrn169
commented
Jun 28, 2020
| * Represents a result of a CheckAndMutate operation | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving |
There was a problem hiding this comment.
Okay. I will remove the IS annotation. Thanks.
| @@ -349,7 +349,7 @@ private void preCheck() { | |||
| loc, stub, mutation, | |||
| (rn, rm) -> RequestConverter.buildMutateRequest(rn, row, family, qualifier, op, value, | |||
| null, timeRange, rm), | |||
| resp -> resp.getExists())) | |||
| resp -> ((CheckAndMutateResult) resp).isSuccess())) | |||
There was a problem hiding this comment.
I will modify it to make use of generic type. Thanks.
| } | ||
|
|
||
| // We need the MultiRequest when constructing the org.apache.hadoop.hbase.client.MultiResponse, | ||
| // so here I write a new method as I do not want to change the abstraction of call method. | ||
| private <RESP> CompletableFuture<RESP> mutateRow(HBaseRpcController controller, | ||
| HRegionLocation loc, ClientService.Interface stub, RowMutations mutation, | ||
| Converter<MultiRequest, byte[], RowMutations> reqConvert, | ||
| Function<Result, RESP> respConverter) { | ||
| Function<Object, RESP> respConverter) { |
There was a problem hiding this comment.
The reason why I changed Result to Object here is that this method is used by both checkAndMutate() and mutateRow() and we need to use different types each case (respectively CheckAndMutateResult and Result). However as you mentioned, we can make use of generic type here. I will modify it. Thanks.
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.
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.
|
I modified the patch for the review and looks the QA is okay. @Apache9 Can you please review this? Thank you in advance. |
|
@Apache9 Please let me know if you have any questions about this. Thank you in advance. |
|
I missed that this had some checkstyle errors. I fixed the errors. |
|
Ping @Apache9. Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
brfrn169
changed the title
Apache9
requested changes
Jul 4, 2020
| return FutureUtils.get(table.checkAndMutate(checkAndMutate)); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean[] checkAndMutate(List<CheckAndMutate> checkAndMutates) throws IOException { | ||
| return Booleans.toArray(FutureUtils.get(table.checkAndMutateAll(checkAndMutates))); | ||
| public CheckAndMutateResult[] checkAndMutate(List<CheckAndMutate> checkAndMutates) |
There was a problem hiding this comment.
Yeah that's better. I will change it.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncTableImpl.java
Show resolved
Hide resolved
brfrn169
commented
Jul 5, 2020
| return FutureUtils.get(table.checkAndMutate(checkAndMutate)); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean[] checkAndMutate(List<CheckAndMutate> checkAndMutates) throws IOException { | ||
| return Booleans.toArray(FutureUtils.get(table.checkAndMutateAll(checkAndMutates))); | ||
| public CheckAndMutateResult[] checkAndMutate(List<CheckAndMutate> checkAndMutates) |
There was a problem hiding this comment.
Yeah that's better. I will change it.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncTableImpl.java
Show resolved
Hide resolved
… introduced in HBASE-8458
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… introduced in HBASE-8458