HBASE-8458 Support for batch version of checkAndPut() and checkAndDel… #1320
Conversation
This file contains hidden or 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
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.
brfrn169
requested review from
Apache9,
ndimiduk and
saintstack
and removed request for
Apache9 and
ndimiduk
HorizonNet
reviewed
Mar 22, 2020
hbase-client/src/main/java/org/apache/hadoop/hbase/CheckAndMutate.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/CheckAndMutate.java
Outdated
Show resolved
Hide resolved
saintstack
reviewed
Mar 25, 2020
hbase-client/src/main/java/org/apache/hadoop/hbase/CheckAndMutate.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public Builder qualifier(byte[] qualifier) { | ||
| this.qualifier = Preconditions.checkNotNull(qualifier, "qualifier is null. Consider using" + | ||
| " an empty byte array, or just do not call this method if you want a null qualifier"); |
There was a problem hiding this comment.
Helpful precondition fail message.
On fail, it comes out ok? Back to the client?
There was a problem hiding this comment.
Back to the client. On fail, it will throw NullPointerException.
| * @param value the expected value | ||
| * @return the builder object | ||
| */ | ||
| public Builder ifMatches(CompareOperator compareOp, byte[] value) { |
There was a problem hiding this comment.
s/ifMatches/ifCompares/?
There was a problem hiding this comment.
We already have the same name as this method in Table$CheckAndMutateBuilder and AsyncTable$CheckAndMutateBuilder, so I named this method as the same name.
Table$CheckAndMutateBuilder#ifMatches:
https://github.com/brfrn169/hbase/blob/f66cbe1a40cd9d4cd4abd27c7ac9d1a5168b885f/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Table.java#L346
AsyncTable$CheckAndMutateBuilder#ifMatches:
https://github.com/brfrn169/hbase/blob/f66cbe1a40cd9d4cd4abd27c7ac9d1a5168b885f/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTable.java#L269
hbase-client/src/main/java/org/apache/hadoop/hbase/CheckAndMutate.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java
Show resolved
Hide resolved
| } | ||
| MutationProto mp = ProtobufUtil.toMutationNoData(type, mutation, mutationBuilder); | ||
| cells.add(mutation); | ||
| regionActionBuilder.addAction(actionBuilder.setMutation(mp).build()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Argh. The old code was hard to decipher.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
Show resolved
Hide resolved
brfrn169
commented
Mar 28, 2020
There was a problem hiding this comment.
Thank you for reviewing this! @saintstack @HorizonNet
I will modify this patch and push a new patch.
| */ | ||
| public Builder qualifier(byte[] qualifier) { | ||
| this.qualifier = Preconditions.checkNotNull(qualifier, "qualifier is null. Consider using" + | ||
| " an empty byte array, or just do not call this method if you want a null qualifier"); |
There was a problem hiding this comment.
Back to the client. On fail, it will throw NullPointerException.
hbase-client/src/main/java/org/apache/hadoop/hbase/CheckAndMutate.java
Outdated
Show resolved
Hide resolved
| * @param value the expected value | ||
| * @return the builder object | ||
| */ | ||
| public Builder ifMatches(CompareOperator compareOp, byte[] value) { |
There was a problem hiding this comment.
We already have the same name as this method in Table$CheckAndMutateBuilder and AsyncTable$CheckAndMutateBuilder, so I named this method as the same name.
Table$CheckAndMutateBuilder#ifMatches:
https://github.com/brfrn169/hbase/blob/f66cbe1a40cd9d4cd4abd27c7ac9d1a5168b885f/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Table.java#L346
AsyncTable$CheckAndMutateBuilder#ifMatches:
https://github.com/brfrn169/hbase/blob/f66cbe1a40cd9d4cd4abd27c7ac9d1a5168b885f/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTable.java#L269
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTable.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/CheckAndMutate.java
Outdated
Show resolved
Hide resolved
| * @param checkAndMutates The list of rows to apply. | ||
| * @return A {@link CompletableFuture} that wrapper the result boolean list. | ||
| */ | ||
| default CompletableFuture<List<Boolean>> checkAndMutateAll( |
There was a problem hiding this comment.
In AsyncTable, for methods returning List object, we provide 2 pattern methods returning List<CompletableFuture> and CompletableFuture<List>, and the latter's name is the former's name + "All".
For example, we already provide AsyncTable#get and AsyncTable#getAll:
https://github.com/brfrn169/hbase/blob/f66cbe1a40cd9d4cd4abd27c7ac9d1a5168b885f/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTable.java#L452-L471
I did the same for checkAndMutate and checkAndMutateAll.
|
I added a new commit. In this commit, I did the following:
@saintstack @Apache9 @HorizonNet Could you please take look at this when you get a chance? |
HorizonNet
reviewed
Mar 30, 2020
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTable.java
Show resolved
Hide resolved
|
I fixed some conflicts and added the Javadoc for the deprecation. Also, I re-ran QA. |
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.
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.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Closing this. Will create another PR. |
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.
…ete()