[GitHub] [hbase] brfrn169 commented on a change in pull request #2094: HBASE-24680 Refactor the checkAndMutate code on the server side

[GitBox <gi...@apache.org>]

brfrn169 commented on a change in pull request #2094:
URL: https://github.com/apache/hbase/pull/2094#discussion_r459957143



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -4272,43 +4275,96 @@ protected Durability getEffectiveDurability(Durability d) {
   }
 
   @Override
+  @Deprecated
   public boolean checkAndMutate(byte[] row, byte[] family, byte[] qualifier, CompareOperator op,
     ByteArrayComparable comparator, TimeRange timeRange, Mutation mutation) throws IOException {
-    return doCheckAndRowMutate(row, family, qualifier, op, comparator, null, timeRange, null,
-      mutation);
+    CheckAndMutate.Builder builder = CheckAndMutate.newBuilder(row)
+      .ifMatches(family, qualifier, op, comparator.getValue()).timeRange(timeRange);
+    try {
+      if (mutation instanceof Put) {
+        return checkAndMutate(builder.build((Put) mutation)).isSuccess();
+      } else if (mutation instanceof Delete) {
+        return checkAndMutate(builder.build((Delete) mutation)).isSuccess();
+      } else {
+        throw new DoNotRetryIOException(
+          "Unsupported mutate type: " + mutation.getClass().getSimpleName().toUpperCase());
+      }
+    } catch (IllegalArgumentException e) {

Review comment:
       CheckAndMutate.Builder.build() calls preCheck() internally and it can throw IllegalArgumentException if the parameters are invalid:
   https://github.com/apache/hbase/blob/09e7ccd42dc0b41241c193e27a7c24e53c8408d4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/CheckAndMutate.java#L141-L151

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -4335,32 +4391,14 @@ private boolean doCheckAndRowMutate(byte[] row, byte[] family, byte[] qualifier,
       checkRow(row, "doCheckAndRowMutate");
       RowLock rowLock = getRowLockInternal(get.getRow(), false, null);
       try {
-        if (mutation != null && this.getCoprocessorHost() != null) {
-          // Call coprocessor.
-          Boolean processed = null;
-          if (mutation instanceof Put) {
-            if (filter != null) {
-              processed = this.getCoprocessorHost()

Review comment:
       The old coprocessor methods will be called after this change because the default implementations of the new coprocessor methods call the old methods for backward compatibility:
   
   RegionObserver#preCheckAndMutate:
   https://github.com/brfrn169/hbase/blob/HBASE-24680/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java#L837-L865
   
   RegionObserver#preCheckAndMutateAfterRowLock:
   https://github.com/brfrn169/hbase/blob/HBASE-24680/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java#L885-L914
   
   RegionObserver#postCheckAndMutate:
   https://github.com/brfrn169/hbase/blob/HBASE-24680/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java#L927-L955
   
   This change doesn't break the existing tests for the old coprocessor methods.

##########
File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java
##########
@@ -86,6 +86,12 @@
    */
   void updateCheckAndPut(long t);
 
+  /**
+   * Update checkAndMutate histogram
+   * @param t time it took
+   */
+  void updateCheckAndMutate(long t);

Review comment:
       Yes. This change keeps the existing metrics for checkAndPut and checkAndDelete for backward compatibility. This change doesn't break the existing tests for the metrics for checkAndPut and checkAndDelete.




----------------------------------------------------------------
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:
users@infra.apache.org