brfrn169 commented on a change in pull request #2228:
URL: https://github.com/apache/hbase/pull/2228#discussion_r475229008
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##########
@@ -742,36 +741,35 @@ private Result increment(final HRegion region, final
OperationQuota quota,
spaceQuota.getPolicyEnforcement(region).check(increment);
quota.addMutation(increment);
Result r = null;
- if (region.getCoprocessorHost() != null) {
- r = region.getCoprocessorHost().preIncrement(increment);
- }
- if (r == null) {
- boolean canProceed = startNonceOperation(mutation, nonceGroup);
- boolean success = false;
- try {
- long nonce = mutation.hasNonce() ? mutation.getNonce() :
HConstants.NO_NONCE;
- if (canProceed) {
- r = region.increment(increment, nonceGroup, nonce);
- } else {
+ boolean canProceed = startNonceOperation(mutation, nonceGroup);
Review comment:
Thank you for taking a look at this! @Apache9
Let me clarify this.
The pseudo codes of before and after the change are as follows:
The pseudo code before the change:
```
r = preIncrement(increment)
if (r == null) {
canProceed = startNonceOperation()
if (canProceed) {
// Process increment...
r = ...
} else {
// Get the incremented cells...
r = ...
}
r = postIncrement(increment, r)
if (canProceed) {
endNonceOperation()
}
}
return r;
```
The pseudo code after the change:
```
canProceed = startNonceOperation()
if (canProceed) {
// The following logic is actually executed in HRegion.batchMutate()
r = preIncrement(increment)
if (r != null) {
// Process increment...
r = ...
r = postIncrement(increment, r)
}
} else {
r = preIncrement(increment)
if (r != null) {
// Get the incremented cells...
r = ...
r = postIncrement(increment, r)
}
}
if (canProceed) {
endNonceOperation()
}
return r
```
I think we can discuss two cases when we don't set any RegionObserver or not.
### The case when we don't set any RegionObserver (preIncrement() always
returns null)
The behavior of the code before the change when startNonceOperation()
returns true:
1. r = preIncrement(increment) -> This returns null
2. canProceed = startNonceOperation() -> This returns true
3. // Process increment...
4. r = postIncrement(increment, r)
5. endNonceOperation()
6. return r
The behavior of the code before the change when startNonceOperation()
returns false:
1. r = preIncrement(increment) -> This returns null
2. canProceed = startNonceOperation() -> This returns false
3. // Get the incremented cells...
4. r = postIncrement(increment, r)
5. return r
The behavior of the code after the change when startNonceOperation() returns
true:
1. canProceed = startNonceOperation() -> This returns true
2. r = preIncrement(increment) -> This returns null
3. // Process increment...
4. r = postIncrement(increment, r)
5. endNonceOperation()
6. return r
The behavior of the code after the change when startNonceOperation() returns
false:
1. canProceed = startNonceOperation() -> This returns false
2. r = preIncrement(increment) -> This returns null
3. // Get the incremented cells...
4. r = postIncrement(increment, r)
5. return r
I don't think the behaviors in this case are essentially changed.
### The case when we set RegionObserver with implementing preIncrement()
The behavior of the code before the change:
1. r = preIncrement(increment) -> This returns a non-null value
2. return r
The behavior of the code after the change when startNonceOperation() returns
true:
1. canProceed = startNonceOperation() -> This returns true
2. r = preIncrement(increment) -> This returns a non-null value
3. endNonceOperation()
4. return r
The behavior of the code after the change when startNonceOperation() returns
false:
1. canProceed = startNonceOperation() -> This returns false
2. r = preIncrement(increment) -> This returns a non-null value
3. return r
The behaviors in this case are a bit changed. After the change, we always
call startNonceOperation(). However, I don't think that affects the result
itself and when we don't set any RegionObserver, we always call
startNonceOperation(), too. So I don't think it's a big problem.
What do you think? Thanks.
----------------------------------------------------------------
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:
[email protected]
