HBASE-22914 Backport HBASE-20662 Increasing space quota on a violated…

… table does not remove SpaceViolationPolicy.DISABLE enforcement (apache#534)

Signed-off-by: Sakthi <sakthi@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
(cherry picked from commit a1161c6)

Change-Id: I436b388089354eaaaf887936e27dbb78e9789f72

hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/QuotaTableUtil.java

@@ -294,6 +294,18 @@ public static Scan makeQuotaSnapshotScanForTable(TableName tn) {
     return s;
   }
 
+  /**
+   * Creates a {@link Get} which returns only {@link SpaceQuotaSnapshot} from the quota table for a
+   * specific table.
+   * @param tn table name to get from. Can't be null.
+   */
+  public static Get makeQuotaSnapshotGetForTable(TableName tn) {
+    Get g = new Get(getTableRowKey(tn));
+    // Limit to "u:v" column
+    g.addColumn(QUOTA_FAMILY_USAGE, QUOTA_QUALIFIER_POLICY);
+    return g;
+  }
+
   /**
    * Extracts the {@link SpaceViolationPolicy} and {@link TableName} from the provided
    * {@link Result} and adds them to the given {@link Map}. If the result does not contain
@@ -306,7 +318,7 @@ public static Scan makeQuotaSnapshotScanForTable(TableName tn) {
   public static void extractQuotaSnapshot(
       Result result, Map<TableName,SpaceQuotaSnapshot> snapshots) {
     byte[] row = Objects.requireNonNull(result).getRow();
-    if (row == null) {
+    if (row == null || row.length == 0) {
       throw new IllegalArgumentException("Provided result had a null row");
     }
     final TableName targetTableName = getTableFromRowKey(row);
@@ -762,6 +774,28 @@ public static Set<String> getNamespaceSnapshots(Connection conn) throws IOExcept
     }
   }
 
+  /*
+   * Returns the current space quota snapshot of the given {@code tableName} from
+   * {@code QuotaTableUtil.QUOTA_TABLE_NAME} or null if the no quota information is available for
+   * that tableName.
+   * @param conn connection to re-use
+   * @param tableName name of the table whose current snapshot is to be retreived
+   */
+  public static SpaceQuotaSnapshot getCurrentSnapshotFromQuotaTable(Connection conn,
+      TableName tableName) throws IOException {
+    try (Table quotaTable = conn.getTable(QuotaTableUtil.QUOTA_TABLE_NAME)) {
+      Map<TableName, SpaceQuotaSnapshot> snapshots = new HashMap<>(1);
+      Result result = quotaTable.get(makeQuotaSnapshotGetForTable(tableName));
+      // if we don't have any row corresponding to this get, return null
+      if (result.isEmpty()) {
+        return null;
+      }
+      // otherwise, extract quota snapshot in snapshots object
+      extractQuotaSnapshot(result, snapshots);
+      return snapshots.get(tableName);
+    }
+  }
+
   /* =========================================================================
    *  Quotas protobuf helpers
    */

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -163,10 +163,14 @@
 import org.apache.hadoop.hbase.quotas.MasterQuotaManager;
 import org.apache.hadoop.hbase.quotas.MasterQuotasObserver;
 import org.apache.hadoop.hbase.quotas.QuotaObserverChore;
+import org.apache.hadoop.hbase.quotas.QuotaTableUtil;
 import org.apache.hadoop.hbase.quotas.QuotaUtil;
 import org.apache.hadoop.hbase.quotas.SnapshotQuotaObserverChore;
+import org.apache.hadoop.hbase.quotas.SpaceQuotaSnapshot;
+import org.apache.hadoop.hbase.quotas.SpaceQuotaSnapshot.SpaceQuotaStatus;
 import org.apache.hadoop.hbase.quotas.SpaceQuotaSnapshotNotifier;
 import org.apache.hadoop.hbase.quotas.SpaceQuotaSnapshotNotifierFactory;
+import org.apache.hadoop.hbase.quotas.SpaceViolationPolicy;
 import org.apache.hadoop.hbase.regionserver.HRegionServer;
 import org.apache.hadoop.hbase.regionserver.RSRpcServices;
 import org.apache.hadoop.hbase.replication.ReplicationException;
@@ -218,8 +222,6 @@
 
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse.CompactionState;
-import org.apache.hadoop.hbase.shaded.protobuf.generated.QuotaProtos.Quotas;
-import org.apache.hadoop.hbase.shaded.protobuf.generated.QuotaProtos.SpaceViolationPolicy;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos.SnapshotDescription;
 
 /**
@@ -2285,10 +2287,12 @@ protected void run() throws IOException {
         MasterQuotaManager quotaManager = getMasterQuotaManager();
         if (quotaManager != null) {
           if (quotaManager.isQuotaInitialized()) {
-            Quotas quotaForTable = QuotaUtil.getTableQuota(getConnection(), tableName);
-            if (quotaForTable != null && quotaForTable.hasSpace()) {
-              SpaceViolationPolicy policy = quotaForTable.getSpace().getViolationPolicy();
-              if (SpaceViolationPolicy.DISABLE == policy) {
+              SpaceQuotaSnapshot currSnapshotOfTable =
+                  QuotaTableUtil.getCurrentSnapshotFromQuotaTable(getConnection(), tableName);
+              if (currSnapshotOfTable != null) {
+                SpaceQuotaStatus quotaStatus = currSnapshotOfTable.getQuotaStatus();
+                if (quotaStatus.isInViolation()
+                    && SpaceViolationPolicy.DISABLE == quotaStatus.getPolicy().orElse(null)) {
                 throw new AccessDeniedException("Enabling the table '" + tableName
                     + "' is disallowed due to a violated space quota.");
               }

hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/MasterQuotaManager.java

@@ -38,6 +38,7 @@
 import org.apache.hadoop.hbase.master.procedure.ProcedurePrepareLatch;
 import org.apache.hadoop.hbase.master.procedure.SwitchRpcThrottleProcedure;
 import org.apache.hadoop.hbase.namespace.NamespaceAuditor;
+import org.apache.hadoop.hbase.quotas.SpaceQuotaSnapshot.SpaceQuotaStatus;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.yetus.audience.InterfaceStability;
@@ -269,7 +270,16 @@ public void update(GlobalQuotaSettingsImpl quotaPojo) throws IOException {
       }
       @Override
       public void delete() throws IOException {
+        SpaceQuotaSnapshot currSnapshotOfTable =
+            QuotaTableUtil.getCurrentSnapshotFromQuotaTable(masterServices.getConnection(), table);
         QuotaUtil.deleteTableQuota(masterServices.getConnection(), table);
+        if (currSnapshotOfTable != null) {
+          SpaceQuotaStatus quotaStatus = currSnapshotOfTable.getQuotaStatus();
+          if (SpaceViolationPolicy.DISABLE == quotaStatus.getPolicy().orElse(null)
+              && quotaStatus.isInViolation()) {
+            QuotaUtil.enableTableIfNotEnabled(masterServices.getConnection(), table);
+          }
+        }
       }
       @Override
       public void preApply(GlobalQuotaSettingsImpl quotaPojo) throws IOException {

hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaObserverChore.java

@@ -188,7 +188,8 @@ void _chore() throws IOException {
 
     for (TableName tableInLimbo : tablesInLimbo) {
       final SpaceQuotaSnapshot currentSnapshot = tableSnapshotStore.getCurrentState(tableInLimbo);
-      if (currentSnapshot.getQuotaStatus().isInViolation()) {
+      SpaceQuotaStatus currentStatus = currentSnapshot.getQuotaStatus();
+      if (currentStatus.isInViolation()) {
         if (LOG.isTraceEnabled()) {
           LOG.trace("Moving " + tableInLimbo + " out of violation because fewer region sizes were"
               + " reported than required.");
@@ -199,6 +200,10 @@ void _chore() throws IOException {
         this.snapshotNotifier.transitionTable(tableInLimbo, targetSnapshot);
         // Update it in the Table QuotaStore so that memory is consistent with no violation.
         tableSnapshotStore.setCurrentState(tableInLimbo, targetSnapshot);
+        // In case of Disable SVP, we need to enable the table as it moves out of violation
+        if (SpaceViolationPolicy.DISABLE == currentStatus.getPolicy().orElse(null)) {
+          QuotaUtil.enableTableIfNotEnabled(conn, tableInLimbo);
+        }
       }
     }
 
@@ -324,20 +329,35 @@ void updateTableQuota(
 
     // If we're changing something, log it.
     if (!currentSnapshot.equals(targetSnapshot)) {
+      this.snapshotNotifier.transitionTable(table, targetSnapshot);
+      // Update it in memory
+      tableSnapshotStore.setCurrentState(table, targetSnapshot);
+
       // If the target is none, we're moving out of violation. Update the hbase:quota table
+      SpaceViolationPolicy currPolicy = currentStatus.getPolicy().orElse(null);
+      SpaceViolationPolicy targetPolicy = targetStatus.getPolicy().orElse(null);
       if (!targetStatus.isInViolation()) {
+        // In case of Disable SVP, we need to enable the table as it moves out of violation
+        if (isDisableSpaceViolationPolicy(currPolicy, targetPolicy)) {
+          QuotaUtil.enableTableIfNotEnabled(conn, table);
+        }
         if (LOG.isDebugEnabled()) {
-          LOG.debug(table + " moving into observance of table space quota.");
+          LOG.debug(table + " moved into observance of table space quota.");
         }
-      } else if (LOG.isDebugEnabled()) {
+      } else {
         // We're either moving into violation or changing violation policies
-        LOG.debug(table + " moving into violation of table space quota with policy of "
-            + targetStatus.getPolicy());
+        if (currPolicy != targetPolicy && SpaceViolationPolicy.DISABLE == currPolicy) {
+          // In case of policy switch, we need to enable the table if current policy is Disable SVP
+          QuotaUtil.enableTableIfNotEnabled(conn, table);
+        } else if (SpaceViolationPolicy.DISABLE == targetPolicy) {
+          // In case of Disable SVP, we need to disable the table as it moves into violation
+          QuotaUtil.disableTableIfNotDisabled(conn, table);
+        }
+        if (LOG.isDebugEnabled()) {
+          LOG.debug(
+            table + " moved into violation of table space quota with policy of " + targetPolicy);
+        }
       }
-
-      this.snapshotNotifier.transitionTable(table, targetSnapshot);
-      // Update it in memory
-      tableSnapshotStore.setCurrentState(table, targetSnapshot);
     } else if (LOG.isTraceEnabled()) {
       // Policies are the same, so we have nothing to do except log this. Don't need to re-update
       // the quota table
@@ -349,6 +369,19 @@ void updateTableQuota(
     }
   }
 
+  /**
+   * Method to check whether we are dealing with DISABLE {@link SpaceViolationPolicy}. In such a
+   * case, currPolicy or/and targetPolicy will be having DISABLE policy.
+   * @param currPolicy currently set space violation policy
+   * @param targetPolicy new space violation policy
+   * @return true if is DISABLE space violation policy; otherwise false
+   */
+  private boolean isDisableSpaceViolationPolicy(final SpaceViolationPolicy currPolicy,
+      final SpaceViolationPolicy targetPolicy) {
+    return SpaceViolationPolicy.DISABLE == currPolicy
+        || SpaceViolationPolicy.DISABLE == targetPolicy;
+  }
+
   /**
    * Updates the hbase:quota table with the target quota policy for this <code>namespace</code>
    * if necessary.
@@ -363,7 +396,7 @@ void updateNamespaceQuota(
       final Multimap<String,TableName> tablesByNamespace) throws IOException {
     final SpaceQuotaStatus targetStatus = targetSnapshot.getQuotaStatus();
 
-    // When the policies differ, we need to move into or out of violatino
+    // When the policies differ, we need to move into or out of violation
     if (!currentSnapshot.equals(targetSnapshot)) {
       // We want to have a policy of "NONE", moving out of violation
       if (!targetStatus.isInViolation()) {

hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaUtil.java

@@ -31,6 +31,9 @@
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.TableNotDisabledException;
+import org.apache.hadoop.hbase.TableNotEnabledException;
+import org.apache.hadoop.hbase.TableNotFoundException;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.client.Delete;
 import org.apache.hadoop.hbase.client.Get;
@@ -500,4 +503,35 @@ public static long calculateResultSize(final List<Result> results) {
     }
     return size;
   }
+
+  /**
+   * Method to enable a table, if not already enabled. This method suppresses
+   * {@link TableNotDisabledException} and {@link TableNotFoundException}, if thrown while enabling
+   * the table.
+   * @param conn connection to re-use
+   * @param tableName name of the table to be enabled
+   */
+  public static void enableTableIfNotEnabled(Connection conn, TableName tableName)
+      throws IOException {
+    try {
+      conn.getAdmin().enableTable(tableName);
+    } catch (TableNotDisabledException | TableNotFoundException e) {
+      // ignore
+    }
+  }
+
+  /**
+   * Method to disable a table, if not already disabled. This method suppresses
+   * {@link TableNotEnabledException}, if thrown while disabling the table.
+   * @param conn connection to re-use
+   * @param tableName table name which has moved into space quota violation
+   */
+  public static void disableTableIfNotDisabled(Connection conn, TableName tableName)
+      throws IOException {
+    try {
+      conn.getAdmin().disableTable(tableName);
+    } catch (TableNotEnabledException | TableNotFoundException e) {
+      // ignore
+    }
+  }
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/SpaceQuotaRefresherChore.java

@@ -96,18 +96,26 @@ protected void chore() {
           LOG.trace(tableName + ": current=" + currentSnapshot + ", new=" + newSnapshot);
         }
         if (!newSnapshot.equals(currentSnapshot)) {
-          // We have a new snapshot. We might need to enforce it or disable the enforcement
-          if (!isInViolation(currentSnapshot) && newSnapshot.getQuotaStatus().isInViolation()) {
+          // We have a new snapshot.
+          // We might need to enforce it or disable the enforcement or switch policy
+          boolean currInViolation = isInViolation(currentSnapshot);
+          boolean newInViolation = newSnapshot.getQuotaStatus().isInViolation();
+          if (!currInViolation && newInViolation) {
             if (LOG.isTraceEnabled()) {
               LOG.trace("Enabling " + newSnapshot + " on " + tableName);
             }
             getManager().enforceViolationPolicy(tableName, newSnapshot);
-          }
-          if (isInViolation(currentSnapshot) && !newSnapshot.getQuotaStatus().isInViolation()) {
+          } else if (currInViolation && !newInViolation) {
             if (LOG.isTraceEnabled()) {
               LOG.trace("Removing quota violation policy on " + tableName);
             }
             getManager().disableViolationPolicyEnforcement(tableName);
+          } else if (currInViolation && newInViolation) {
+            if (LOG.isTraceEnabled()) {
+              LOG.trace("Switching quota violation policy on " + tableName + " from "
+                  + currentSnapshot + " to " + newSnapshot);
+            }
+            getManager().enforceViolationPolicy(tableName, newSnapshot);
           }
         }
       }

hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/policies/DisableTableViolationPolicyEnforcement.java

@@ -18,55 +18,29 @@
 
 import java.io.IOException;
 
-import org.apache.hadoop.hbase.TableNotDisabledException;
-import org.apache.hadoop.hbase.TableNotEnabledException;
-import org.apache.hadoop.hbase.TableNotFoundException;
 import org.apache.yetus.audience.InterfaceAudience;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.apache.hadoop.hbase.client.Mutation;
 import org.apache.hadoop.hbase.quotas.SpaceLimitingException;
 import org.apache.hadoop.hbase.quotas.SpaceViolationPolicy;
 import org.apache.hadoop.hbase.quotas.SpaceViolationPolicyEnforcement;
 
 /**
- * A {@link SpaceViolationPolicyEnforcement} which disables the table. The enforcement
- * counterpart to {@link SpaceViolationPolicy#DISABLE}.
+ * A {@link SpaceViolationPolicyEnforcement} which disables the table. The enforcement counterpart
+ * to {@link SpaceViolationPolicy#DISABLE}. This violation policy is different from others as it
+ * doesn't take action (i.e. enable/disable table) local to the RegionServer, like the other
+ * ViolationPolicies do. In case of violation, the appropriate action is initiated by the master.
  */
 @InterfaceAudience.Private
 public class DisableTableViolationPolicyEnforcement extends DefaultViolationPolicyEnforcement {
-  private static final Logger LOG =
-      LoggerFactory.getLogger(DisableTableViolationPolicyEnforcement.class);
 
   @Override
   public void enable() throws IOException {
-    try {
-      if (LOG.isTraceEnabled()) {
-        LOG.trace("Starting disable of " + getTableName());
-      }
-      getRegionServerServices().getClusterConnection().getAdmin().disableTable(getTableName());
-      if (LOG.isTraceEnabled()) {
-        LOG.trace("Disable is complete for " + getTableName());
-      }
-    } catch (TableNotEnabledException tnee) {
-      // The state we wanted it to be in.
-    }
+    // do nothing
   }
 
   @Override
   public void disable() throws IOException {
-    try {
-      if (LOG.isTraceEnabled()) {
-        LOG.trace("Starting enable of " + getTableName());
-      }
-      getRegionServerServices().getClusterConnection().getAdmin().enableTable(getTableName());
-      if (LOG.isTraceEnabled()) {
-        LOG.trace("Enable is complete for " + getTableName());
-      }
-    } catch (TableNotDisabledException | TableNotFoundException e) {
-      // The state we wanted it to be in
-      // Or, in case table is not found, nothing to do
-    }
+    // do nothing
   }
 
   @Override