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

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

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



##########
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:
       After this change, do we still have checkAndPut and checkAndDelete?

##########
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) {
+      throw new DoNotRetryIOException(e.getMessage());
+    }
   }
 
   @Override
+  @Deprecated
   public boolean checkAndMutate(byte[] row, Filter filter, TimeRange timeRange, Mutation mutation)
     throws IOException {
-    return doCheckAndRowMutate(row, null, null, null, null, filter, timeRange, null, mutation);
+    CheckAndMutate.Builder builder = CheckAndMutate.newBuilder(row).ifMatches(filter)
+      .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:
       Ditto.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
##########
@@ -759,12 +810,150 @@ default boolean postCheckAndDelete(ObserverContext<RegionCoprocessorEnvironment>
    * @param delete delete to commit if check succeeds
    * @param result from the CheckAndDelete
    * @return the possibly transformed returned value to return to client
+   *
+   * @deprecated since 3.0.0 and will be removed in 4.0.0. Use
+   *   {@link #postCheckAndMutate(ObserverContext, CheckAndMutate, CheckAndMutateResult)} instead.
    */
+  @Deprecated
   default boolean postCheckAndDelete(ObserverContext<RegionCoprocessorEnvironment> c, byte[] row,
     Filter filter, Delete delete, boolean result) throws IOException {
     return result;
   }
 
+  /**
+   * Called before checkAndMutate
+   * <p>
+   * Call CoprocessorEnvironment#bypass to skip default actions.
+   * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors.
+   * <p>
+   * Note: Do not retain references to any Cells in actions beyond the life of this invocation.
+   * If need a Cell reference for later use, copy the cell and use that.
+   * @param c the environment provided by the region server
+   * @param checkAndMutate the CheckAndMutate object
+   * @param result the default value of the result
+   * @return the return value to return to client if bypassing default processing
+   * @throws IOException if an error occurred on the coprocessor
+   */
+  default CheckAndMutateResult preCheckAndMutate(ObserverContext<RegionCoprocessorEnvironment> c,
+    CheckAndMutate checkAndMutate, CheckAndMutateResult result) throws IOException {
+    if (checkAndMutate.getAction() instanceof Put) {

Review comment:
       Put/Delete is public so let's not do this...

##########
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:
       So the old coprocessor methods will not be called any more?

##########
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:
       Where do we throw this exception out? Catching a RuntimeException seems strange to me.




----------------------------------------------------------------
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