HBASE-24996 Support CheckAndMutate in Region.batchMutate() #2498
Conversation
brfrn169
commented
Oct 4, 2020
•
brfrn169
commented
- After this change, Region.batchMutate() will support CheckAndMutate operations and we will be able to perform Put/Delete/Increment/Append/CheckAndMutate operations atomically.
- When CheckAndMutate operations are executed, the following coprocessor methods of RegionObserver will be no longer called. However, postIncrementBeforeWAL/postAppendBeforeWAL will be still called.
- prePut
- postPut
- preDelete
- postDelete
- preIncrement
- preIncrementAfterRowLock
- postIncrement
- preAppend
- preAppendAfterRowLock
- postAppend
- Also, the following coprocessor methods of RegionObserver will be called when CheckAndMutate operations are performed:
- preBatchMutate()
- postBatchMutate()
- postBatchMutateIndispensably()
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.
| @@ -139,7 +139,8 @@ public Configuration getConfiguration() { | |||
| */ | |||
| @Override | |||
| protected KeyValueScanner createScanner(Scan scan, ScanInfo scanInfo, | |||
| NavigableSet<byte[]> targetCols, long readPt) throws IOException { | |||
| NavigableSet<byte[]> targetCols, long readPt,List<KeyValueScanner> additionalScanners) | |||
There was a problem hiding this comment.
Has to be a KVScanner? Can't be a CellScanner? (Took a look... seems like the latter is something else...)
There was a problem hiding this comment.
It Has to be a KVScanner. I will explain the reason in the following comment:
#2498 (comment)
| @@ -3069,27 +3073,33 @@ private RegionScannerImpl getScanner(Scan scan, List<KeyValueScanner> additional | |||
| checkFamily(family); | |||
| } | |||
| } | |||
| return instantiateRegionScanner(scan, additionalScanners, nonceGroup, nonce); | |||
| return instantiateRegionScanner(scan, additionalScanners, additionalScannersForStores, | |||
There was a problem hiding this comment.
Whats the diff between the two additional Scanner?
There was a problem hiding this comment.
Let me explain this.
There was one requirement where the additional cells of a mutation need to be visible for followed mutations. For example, when we have a Put operation and a CheckAndMutate operation in a single mutation batch, the additional cells of the Put operation should be visible for the followed CheckAndMutate operation.
The following code keeps additional cells of mutations:
https://github.com/brfrn169/hbase/blob/HBASE-24996/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java#L3833-L3835
To achieve this, I first tried to use the existing additionalScanners of RegionScannerImpl:
https://github.com/brfrn169/hbase/blob/HBASE-24996/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java#L3061
However, I found it's much easier to add a new additionalScanners to StoreScanner and use it than to use the existing additionalScanners of RegionScannerImpl.
So, I decided to add additionalScanners to StoreScanner and was able to make the additional cell visible for the followed mutations.
https://github.com/brfrn169/hbase/blob/HBASE-24996/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java#L251-L253
By the way, the test code of this is as as follows:
https://github.com/brfrn169/hbase/blob/HBASE-24996/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java#L7101-L7174
Thanks.
There was a problem hiding this comment.
How do process increment and append in the past? What if we have a put and then an increment on the same row before this PR?
There was a problem hiding this comment.
We didn't handle this situation in the past. So if we have a put and then an increment on the same row, it acts like the put is ignored.
There was a problem hiding this comment.
So let's not mix things up in a single PR? Open another PR to discuss this first? I do not think this is the only way to do this. For HBase, it is not like the traditional RDBMS, where we issue a start transaction command to the server first, and then apply several SQL statements, and last issue a commit transaction command. Here we know all the action before actually executing them, so if there are multiple actions operation on the same row, we could merge them first. For example, if there are two increment on the same row, we could just add the values to merge them into one increment. And if there is a CAS and then a Put, we could just remove the CAS as the internal value is not visible right? And if there is a Put and then a CAS, it is easy to know whether the CAS can be performed, and can even convert it to a Put.
Or maybe even make a simple statement that, please do not operate on the same row in a batch, we will not consider the previous operation in the same batch when performing the operations in the batch.
There was a problem hiding this comment.
Here we know all the action before actually executing them, so if there are multiple actions operation on the same row, we could merge them first.
I thought the same thing first, but it is not such a simple thing because we can specify timestamp to mutations. So we can't always know the latest column value without merging the existing values (in memstore and hfiles). For example, if we have one existing Put with timestamp 2, and if we perform Put with timestamp 1 and CAS for the same column in a batch, we can't know the latest column value without merging the existing values. That's why I needed to add the new additional Scanner. However, I know it will make things complicated.
Or maybe even make a simple statement that, please do not operate on the same row in a batch, we will not consider the previous operation in the same batch when performing the operations in the batch.
Sounds this is a good idea. I will modify the patch this way. Thank you for discussing this.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Outdated
Show resolved
Hide resolved
| // Sort the cells so that they match the order that they appear in the Get results. | ||
| // Otherwise, we won't be able to find the existing values if the cells are not specified | ||
| // in order by the client since cells are in an array list. | ||
| // TODO: I don't get why we are sorting. St.Ack 20150107 |
There was a problem hiding this comment.
Anything on this sort issue?
There was a problem hiding this comment.
We need the sorting actually, otherwise it will return a wrong result to the client. I keep this sorting here: https://github.com/brfrn169/hbase/blob/HBASE-24996/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java#L4072-L4075
| * <p> | ||
| * Note this supports only Put, Delete, Increment and Append mutations and will ignore other | ||
| * types passed. | ||
| * |
|
@saintstack Thank you very much for reviewing this! I'm working on fixing the test failure in the last QA. Once it's done, will work on your review. 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.
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 don't think the test failure in the last QA is related to this change. I will re-trigger the 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.
|
@saintstack @Apache9 @joshelser Could you please review this when you have time? I wrote down the summary of this change in the following comment: Thank you in advance. |
|
@saintstack @Apache9 @joshelser Could you please review this? I want to include this to hbase-2.4 if possible. Thanks. |
|
Sorry for the late reply... This is a big change, need to find a suitable large block of time to review it... Will do this soon this week. |
|
Thank you very much! @Apache9 |
| } | ||
|
|
||
| @Override | ||
| public String getId() { |
There was a problem hiding this comment.
Since we have so many unsupported methods, is it still a good idea to make CheckAndMutate extend Mutation?
There was a problem hiding this comment.
I want to keep CheckAndMutate extend Mutation because I want to treat CheckAndMutate as same as other mutations (Put/Delete/Increment/Append) to perform CheckAndMutate and other mutations atomically.
For example, after this change, we will be able to perform Put and Increment and CheckAndMutate atomically in a single row (or region).
However, I'm thinking that we need to rearrange/redesign the mutation structure in the future.
| @@ -3069,27 +3073,33 @@ private RegionScannerImpl getScanner(Scan scan, List<KeyValueScanner> additional | |||
| checkFamily(family); | |||
| } | |||
| } | |||
| return instantiateRegionScanner(scan, additionalScanners, nonceGroup, nonce); | |||
| return instantiateRegionScanner(scan, additionalScanners, additionalScannersForStores, | |||
There was a problem hiding this comment.
How do process increment and append in the past? What if we have a put and then an increment on the same row before this PR?
There was a problem hiding this comment.
@Apache9 Thank you for taking a look at this!
I'm a bit confused that why we need to touch so many existing code? I suppose what we need to do is to just add a new instanceof branch in the batchMutate method?
Sorry for the confusion. I did multiple things in this commit actually. One of the biggest changes is things related to the additional Scanner. As I mentioned in the other comment, there was the following requirement so I needed to add the new additional Scanner:
There was one requirement where the additional cells of a mutation need to be visible for followed mutations. For example, when we have a Put operation and a CheckAndMutate operation in a single mutation batch, the additional cells of the Put operation should be visible for the followed CheckAndMutate operation.
Another big change is the following as mentioned in #2498 (comment) :
When CheckAndMutate operations are executed, the following coprocessor methods of RegionObserver will be no longer called. However, postIncrementBeforeWAL/postAppendBeforeWAL will be still called.
prePut
postPut
preDelete
postDelete
preIncrement
preIncrementAfterRowLock
postIncrement
preAppend
preAppendAfterRowLock
postAppend
This is a side-effect of this change and I also needed to change AccessController and VisibilityController due to it.
Thanks.
| } | ||
|
|
||
| @Override | ||
| public String getId() { |
There was a problem hiding this comment.
I want to keep CheckAndMutate extend Mutation because I want to treat CheckAndMutate as same as other mutations (Put/Delete/Increment/Append) to perform CheckAndMutate and other mutations atomically.
For example, after this change, we will be able to perform Put and Increment and CheckAndMutate atomically in a single row (or region).
However, I'm thinking that we need to rearrange/redesign the mutation structure in the future.
| @@ -3069,27 +3073,33 @@ private RegionScannerImpl getScanner(Scan scan, List<KeyValueScanner> additional | |||
| checkFamily(family); | |||
| } | |||
| } | |||
| return instantiateRegionScanner(scan, additionalScanners, nonceGroup, nonce); | |||
| return instantiateRegionScanner(scan, additionalScanners, additionalScannersForStores, | |||
There was a problem hiding this comment.
We didn't handle this situation in the past. So if we have a put and then an increment on the same row, it acts like the put is ignored.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The failed UT is not related to the patch. It was successful locally. I removed the new additional Scanner things from the patch and added the following document to the Region.batchMutate method: Can you please review this? @Apache9 |
|
It looks like this change will make things too complicated and it has big side effects and incompatible changes. So I will give it up. Closing it. |
