HBASE-28151 Option to allow/disallow bypassing pre transit check for assing/unassign

hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseHbck.java

@@ -135,10 +135,11 @@ public Map<String, RegionState.State> setRegionStateInMeta(
   }
 
   @Override
-  public List<Long> assigns(List<String> encodedRegionNames, boolean override) throws IOException {
+  public List<Long> assigns(List<String> encodedRegionNames, boolean override, boolean force)
+    throws IOException {
     try {
       AssignsResponse response = this.hbck.assigns(rpcControllerFactory.newController(),
-        RequestConverter.toAssignRegionsRequest(encodedRegionNames, override));
+        RequestConverter.toAssignRegionsRequest(encodedRegionNames, override, force));
       return response.getPidList();
     } catch (ServiceException se) {
       LOG.debug(toCommaDelimitedString(encodedRegionNames), se);
@@ -147,11 +148,11 @@ public List<Long> assigns(List<String> encodedRegionNames, boolean override) thr
   }
 
   @Override
-  public List<Long> unassigns(List<String> encodedRegionNames, boolean override)
+  public List<Long> unassigns(List<String> encodedRegionNames, boolean override, boolean force)
     throws IOException {
     try {
       UnassignsResponse response = this.hbck.unassigns(rpcControllerFactory.newController(),
-        RequestConverter.toUnassignRegionsRequest(encodedRegionNames, override));
+        RequestConverter.toUnassignRegionsRequest(encodedRegionNames, override, force));
       return response.getPidList();
     } catch (ServiceException se) {
       LOG.debug(toCommaDelimitedString(encodedRegionNames), se);

hbase-client/src/main/java/org/apache/hadoop/hbase/client/Hbck.java

@@ -62,39 +62,54 @@ public interface Hbck extends Abortable, Closeable {
    * good if many Regions to online -- and it will schedule the assigns even in the case where
    * Master is initializing (as long as the ProcedureExecutor is up). Does NOT call Coprocessor
    * hooks.
-   * @param override           You need to add the override for case where a region has previously
-   *                           been bypassed. When a Procedure has been bypassed, a Procedure will
-   *                           have completed but no other Procedure will be able to make progress
-   *                           on the target entity (intentionally). This override flag will
-   *                           override this fencing mechanism.
+   * @param override           You need to add override for unset of the procedure from
+   *                           RegionStateNode without byPassing preTransitCheck
+   * @param force              You need to add force for case where a region has previously been
+   *                           bypassed. When a Procedure has been bypassed, a Procedure will have
+   *                           completed but no other Procedure will be able to make progress on the
+   *                           target entity (intentionally). Skips preTransitCheck only when
+   *                           selected along with override option
    * @param encodedRegionNames Region encoded names; e.g. 1588230740 is the hard-coded encoding for
    *                           hbase:meta region and de00010733901a05f5a2a3a382e27dd4 is an example
    *                           of what a random user-space encoded Region name looks like.
    */
-  List<Long> assigns(List<String> encodedRegionNames, boolean override) throws IOException;
+  List<Long> assigns(List<String> encodedRegionNames, boolean override, boolean force)
+    throws IOException;
+
+  default List<Long> assigns(List<String> encodedRegionNames, boolean override) throws IOException {
+    return assigns(encodedRegionNames, override, true);
+  }
 
   default List<Long> assigns(List<String> encodedRegionNames) throws IOException {
-    return assigns(encodedRegionNames, false);
+    return assigns(encodedRegionNames, false, false);
   }
 
   /**
    * Like {@link Admin#unassign(byte[], boolean)} but 'raw' in that it can do more than one Region
    * at a time -- good if many Regions to offline -- and it will schedule the assigns even in the
    * case where Master is initializing (as long as the ProcedureExecutor is up). Does NOT call
    * Coprocessor hooks.
-   * @param override           You need to add the override for case where a region has previously
-   *                           been bypassed. When a Procedure has been bypassed, a Procedure will
-   *                           have completed but no other Procedure will be able to make progress
-   *                           on the target entity (intentionally). This override flag will
-   *                           override this fencing mechanism.
+   * @param override           You need to add override for unset of the procedure from
+   *                           RegionStateNode without byPassing preTransitCheck
+   * @param force              You need to add force for case where a region has previously been
+   *                           bypassed. When a Procedure has been bypassed, a Procedure will have
+   *                           completed but no other Procedure will be able to make progress on the
+   *                           target entity (intentionally). Skips preTransitCheck only when
+   *                           selected along with override option
    * @param encodedRegionNames Region encoded names; e.g. 1588230740 is the hard-coded encoding for
    *                           hbase:meta region and de00010733901a05f5a2a3a382e27dd4 is an example
    *                           of what a random user-space encoded Region name looks like.
    */
-  List<Long> unassigns(List<String> encodedRegionNames, boolean override) throws IOException;
+  List<Long> unassigns(List<String> encodedRegionNames, boolean override, boolean force)
+    throws IOException;
+
+  default List<Long> unassigns(List<String> encodedRegionNames, boolean override)
+    throws IOException {
+    return unassigns(encodedRegionNames, override, true);
+  }
 
   default List<Long> unassigns(List<String> encodedRegionNames) throws IOException {
-    return unassigns(encodedRegionNames, false);
+    return unassigns(encodedRegionNames, false, true);
   }
 
   /**

hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java

@@ -1590,17 +1590,17 @@ private static List<HBaseProtos.ServerName> toProtoServerNames(List<ServerName>
 
   // HBCK2
   public static MasterProtos.AssignsRequest toAssignRegionsRequest(List<String> encodedRegionNames,
-    boolean override) {
+    boolean override, boolean force) {
     MasterProtos.AssignsRequest.Builder b = MasterProtos.AssignsRequest.newBuilder();
     return b.addAllRegion(toEncodedRegionNameRegionSpecifiers(encodedRegionNames))
-      .setOverride(override).build();
+      .setOverride(override).setForce(force).build();
   }
 
   public static MasterProtos.UnassignsRequest
-    toUnassignRegionsRequest(List<String> encodedRegionNames, boolean override) {
+    toUnassignRegionsRequest(List<String> encodedRegionNames, boolean override, boolean force) {
     MasterProtos.UnassignsRequest.Builder b = MasterProtos.UnassignsRequest.newBuilder();
     return b.addAllRegion(toEncodedRegionNameRegionSpecifiers(encodedRegionNames))
-      .setOverride(override).build();
+      .setOverride(override).setForce(force).build();
   }
 
   public static MasterProtos.ScheduleServerCrashProcedureRequest

hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto

@@ -1302,6 +1302,7 @@ message SetRegionStateInMetaResponse {
 message AssignsRequest {
   repeated RegionSpecifier region = 1;
   optional bool override = 2 [default = false];
+  optional bool force = 3 [default = false];
 }
 
 /** Like Admin's AssignRegionResponse except it can
@@ -1317,6 +1318,7 @@ message AssignsResponse {
 message UnassignsRequest {
   repeated RegionSpecifier region = 1;
   optional bool override = 2 [default = false];
+  optional bool force= 3 [default = false];
 }
 
 /** Like Admin's UnassignRegionResponse except it can

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

@@ -2714,14 +2714,15 @@ public MasterProtos.AssignsResponse assigns(RpcController controller,
     MasterProtos.AssignsResponse.Builder responseBuilder =
       MasterProtos.AssignsResponse.newBuilder();
     final boolean override = request.getOverride();
+    final boolean force = request.getForce();
     LOG.info("{} assigns, override={}", server.getClientIdAuditPrefix(), override);
     for (HBaseProtos.RegionSpecifier rs : request.getRegionList()) {
       final RegionInfo info = getRegionInfo(rs);
       if (info == null) {
         LOG.info("Unknown region {}", rs);
         continue;
       }
-      responseBuilder.addPid(Optional.ofNullable(am.createOneAssignProcedure(info, override))
+      responseBuilder.addPid(Optional.ofNullable(am.createOneAssignProcedure(info, override, force))
         .map(pe::submitProcedure).orElse(Procedure.NO_PROC_ID));
     }
     return responseBuilder.build();
@@ -2741,15 +2742,17 @@ public MasterProtos.UnassignsResponse unassigns(RpcController controller,
     MasterProtos.UnassignsResponse.Builder responseBuilder =
       MasterProtos.UnassignsResponse.newBuilder();
     final boolean override = request.getOverride();
+    final boolean force = request.getForce();
     LOG.info("{} unassigns, override={}", server.getClientIdAuditPrefix(), override);
     for (HBaseProtos.RegionSpecifier rs : request.getRegionList()) {
       final RegionInfo info = getRegionInfo(rs);
       if (info == null) {
         LOG.info("Unknown region {}", rs);
         continue;
       }
-      responseBuilder.addPid(Optional.ofNullable(am.createOneUnassignProcedure(info, override))
-        .map(pe::submitProcedure).orElse(Procedure.NO_PROC_ID));
+      responseBuilder
+        .addPid(Optional.ofNullable(am.createOneUnassignProcedure(info, override, force))
+          .map(pe::submitProcedure).orElse(Procedure.NO_PROC_ID));
     }
     return responseBuilder.build();
   }

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

@@ -765,11 +765,14 @@ private void preTransitCheck(RegionStateNode regionNode, RegionState.State[] exp
    * @param override If false, check RegionState is appropriate for assign; if not throw exception.
    */
   private TransitRegionStateProcedure createAssignProcedure(RegionInfo regionInfo, ServerName sn,
-    boolean override) throws IOException {
+    boolean override, boolean force) throws IOException {
     RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(regionInfo);
     regionNode.lock();
     try {
       if (override) {
+        if (!force) {
+          preTransitCheck(regionNode, STATES_EXPECTED_ON_ASSIGN);
+        }
         if (regionNode.getProcedure() != null) {
           regionNode.unsetProcedure(regionNode.getProcedure());
         }
@@ -787,7 +790,7 @@ private TransitRegionStateProcedure createAssignProcedure(RegionInfo regionInfo,
   /**
    * Create an assign TransitRegionStateProcedure. Does NO checking of RegionState. Presumes
    * appriopriate state ripe for assign.
-   * @see #createAssignProcedure(RegionInfo, ServerName, boolean)
+   * @see #createAssignProcedure(RegionInfo, ServerName, boolean, boolean)
    */
   private TransitRegionStateProcedure createAssignProcedure(RegionStateNode regionNode,
     ServerName targetServer) {
@@ -801,7 +804,7 @@ private TransitRegionStateProcedure createAssignProcedure(RegionStateNode region
   }
 
   public long assign(RegionInfo regionInfo, ServerName sn) throws IOException {
-    TransitRegionStateProcedure proc = createAssignProcedure(regionInfo, sn, false);
+    TransitRegionStateProcedure proc = createAssignProcedure(regionInfo, sn, false, false);
     ProcedureSyncWait.submitAndWaitProcedure(master.getMasterProcedureExecutor(), proc);
     return proc.getProcId();
   }
@@ -817,7 +820,7 @@ public long assign(RegionInfo regionInfo) throws IOException {
    */
   public Future<byte[]> assignAsync(RegionInfo regionInfo, ServerName sn) throws IOException {
     return ProcedureSyncWait.submitProcedure(master.getMasterProcedureExecutor(),
-      createAssignProcedure(regionInfo, sn, false));
+      createAssignProcedure(regionInfo, sn, false, false));
   }
 
   /**
@@ -961,10 +964,11 @@ static int compare(TransitRegionStateProcedure left, TransitRegionStateProcedure
    * method is called from HBCK2.
    * @return an assign or null
    */
-  public TransitRegionStateProcedure createOneAssignProcedure(RegionInfo ri, boolean override) {
+  public TransitRegionStateProcedure createOneAssignProcedure(RegionInfo ri, boolean override,
+    boolean force) {
     TransitRegionStateProcedure trsp = null;
     try {
-      trsp = createAssignProcedure(ri, null, override);
+      trsp = createAssignProcedure(ri, null, override, force);
     } catch (IOException ioe) {
       LOG.info(
         "Failed {} assign, override={}"
@@ -978,12 +982,16 @@ public TransitRegionStateProcedure createOneAssignProcedure(RegionInfo ri, boole
    * Create one TransitRegionStateProcedure to unassign a region. This method is called from HBCK2.
    * @return an unassign or null
    */
-  public TransitRegionStateProcedure createOneUnassignProcedure(RegionInfo ri, boolean override) {
+  public TransitRegionStateProcedure createOneUnassignProcedure(RegionInfo ri, boolean override,
+    boolean force) {
     RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(ri);
     TransitRegionStateProcedure trsp = null;
     regionNode.lock();
     try {
       if (override) {
+        if (!force) {
+          preTransitCheck(regionNode, STATES_EXPECTED_ON_UNASSIGN_OR_MOVE);
+        }
         if (regionNode.getProcedure() != null) {
           regionNode.unsetProcedure(regionNode.getProcedure());
         }

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

@@ -210,10 +210,10 @@ public TableOperationType getTableOperationType() {
 
   private TransitRegionStateProcedure createUnAssignProcedures(MasterProcedureEnv env)
     throws IOException {
-    return env.getAssignmentManager().createOneUnassignProcedure(getRegion(), true);
+    return env.getAssignmentManager().createOneUnassignProcedure(getRegion(), true, true);
   }
 
   private TransitRegionStateProcedure createAssignProcedures(MasterProcedureEnv env) {
-    return env.getAssignmentManager().createOneAssignProcedure(getRegion(), true);
+    return env.getAssignmentManager().createOneAssignProcedure(getRegion(), true, true);
   }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestHbck.java

@@ -250,11 +250,20 @@ public void testAssigns() throws Exception {
       for (long pid : pids) {
         assertEquals(Procedure.NO_PROC_ID, pid);
       }
-      // If we pass override, then we should be able to unassign EVEN THOUGH Regions already
+      // Rerun the unassign with override. Should fail for all Regions since they already
+      // unassigned; failed
+      // unassign will manifest as all pids being -1 (ever since HBASE-24885).
+      pids = hbck.unassigns(
+        regions.stream().map(RegionInfo::getEncodedName).collect(Collectors.toList()), true, false);
+      waitOnPids(pids);
+      for (long pid : pids) {
+        assertEquals(Procedure.NO_PROC_ID, pid);
+      }
+      // If we pass force, then we should be able to unassign EVEN THOUGH Regions already
       // unassigned.... makes for a mess but operator might want to do this at an extreme when
       // doing fixup of broke cluster.
       pids = hbck.unassigns(
-        regions.stream().map(RegionInfo::getEncodedName).collect(Collectors.toList()), true);
+        regions.stream().map(RegionInfo::getEncodedName).collect(Collectors.toList()), true, true);
       waitOnPids(pids);
       for (long pid : pids) {
         assertNotEquals(Procedure.NO_PROC_ID, pid);
@@ -283,6 +292,12 @@ public void testAssigns() throws Exception {
         LOG.info("RS: {}", rs.toString());
         assertTrue(rs.toString(), rs.isOpened());
       }
+      // Rerun the assign with override. Should fail for all Regions since they already assigned
+      pids = hbck.assigns(
+        regions.stream().map(RegionInfo::getEncodedName).collect(Collectors.toList()), true, false);
+      for (long pid : pids) {
+        assertEquals(Procedure.NO_PROC_ID, pid);
+      }
       // What happens if crappy region list passed?
       pids = hbck.assigns(
         Arrays.stream(new String[] { "a", "some rubbish name" }).collect(Collectors.toList()));

hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionBypass.java

@@ -125,7 +125,7 @@ public void testBypass() throws IOException, InterruptedException {
       .getMasterProcedureExecutor().getActiveProcIds().isEmpty());
     // Now assign with the override flag.
     for (RegionInfo ri : regions) {
-      TEST_UTIL.getHbck().assigns(Arrays.<String> asList(ri.getEncodedName()), true);
+      TEST_UTIL.getHbck().assigns(Arrays.<String> asList(ri.getEncodedName()), true, true);
     }
     TEST_UTIL.waitFor(60000, () -> TEST_UTIL.getHBaseCluster().getMaster()
       .getMasterProcedureExecutor().getActiveProcIds().isEmpty());