HBASE-24885 STUCK RIT by hbck2 assigns (#2283)

Adds region state check on hbck2 assigns/unassigns. Returns pid of -1
if in inappropriate state with logging explaination which suggests
passing override if operator wants to assign/unassign anyways. Here
is an example of what happens now if hbck2 tries an unassign and
Region already unassigned:

  2020-08-19 11:22:06,926 INFO  [RpcServer.default.FPBQ.Fifo.handler=1,queue=0,port=50086] assignment.AssignmentManager(820): Failed {ENCODED => d1112e553991e938b6852f87774c91ee, NAME => 'TestHbck,zzzzz,1597861310769.d1112e553991e938b6852f87774c91ee.', STARTKEY => 'zzzzz', ENDKEY => ''} unassign, override=false; set override to by-pass state checks.
  org.apache.hadoop.hbase.client.DoNotRetryRegionException: Unexpected state for state=CLOSED, location=null, table=TestHbck, region=d1112e553991e938b6852f87774c91ee
          at org.apache.hadoop.hbase.master.assignment.AssignmentManager.preTransitCheck(AssignmentManager.java:583)
          at org.apache.hadoop.hbase.master.assignment.AssignmentManager.createOneUnassignProcedure(AssignmentManager.java:812)
          at org.apache.hadoop.hbase.master.MasterRpcServices.unassigns(MasterRpcServices.java:2616)
          at org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos$HbckService$2.callBlockingMethod(MasterProtos.java)
          at org.apache.hadoop.hbase.ipc.RpcServer.call(RpcServer.java:397)
          at org.apache.hadoop.hbase.ipc.CallRunner.run(CallRunner.java:133)
          at org.apache.hadoop.hbase.ipc.RpcExecutor$Handler.run(RpcExecutor.java:338)
          at org.apache.hadoop.hbase.ipc.RpcExecutor$Handler.run(RpcExecutor.java:318)

Previous it would just create the unassign anyways. Now must pass override
to queue the procedure regardless. Safer.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
 javadoc on assigns/unassigns. Minor refactor in assigns/unassigns to cater to
 case where procedure may come back null (if override not set and fails state checks).

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
 checkstyle cleanups.
 Clarifying javadoc on how there is no state checking when bulk assigns creating/enabling
 tables.

 createOneAssignProcedure and createOneUnassignProcedure now handle exceptions which now
 can be thrown if no override and region state is not appropriate.

 Aggregation of createAssignProcedure and createUnassignProcedure instances adding in
 region state check invoked if override is NOT set.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateNode.java
 Change to setProcedure so it returns passed proc as result instead of void

Signed-off-by: Duo Zhang <zhangduo@apache.org>

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

@@ -73,6 +73,7 @@ import org.apache.hadoop.hbase.ipc.RpcServerInterface;
 import org.apache.hadoop.hbase.ipc.ServerNotRunningYetException;
 import org.apache.hadoop.hbase.ipc.ServerRpcController;
 import org.apache.hadoop.hbase.master.assignment.RegionStates;
+import org.apache.hadoop.hbase.master.assignment.TransitRegionStateProcedure;
 import org.apache.hadoop.hbase.master.locking.LockProcedure;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureUtil;
@@ -2559,30 +2560,40 @@ public class MasterRpcServices extends RSRpcServices implements
   }
 
   /**
-   * A 'raw' version of assign that does bulk and skirts Master state checks (assigns can be made
+   * @throws ServiceException If no MasterProcedureExecutor
-   * during Master startup). For use by Hbck2.
    */
-  @Override
+  private void checkMasterProcedureExecutor() throws ServiceException {
-  public MasterProtos.AssignsResponse assigns(RpcController controller,
-      MasterProtos.AssignsRequest request)
-    throws ServiceException {
     if (this.master.getMasterProcedureExecutor() == null) {
       throw new ServiceException("Master's ProcedureExecutor not initialized; retry later");
     }
+  }
+
+  /**
+   * A 'raw' version of assign that does bulk and can skirt Master state checks if override
+   * is set; i.e. assigns can be forced during Master startup or if RegionState is unclean.
+   * Used by HBCK2.
+   */
+  @Override
+  public MasterProtos.AssignsResponse assigns(RpcController controller,
+      MasterProtos.AssignsRequest request) throws ServiceException {
+    checkMasterProcedureExecutor();
     MasterProtos.AssignsResponse.Builder responseBuilder =
       MasterProtos.AssignsResponse.newBuilder();
     try {
       boolean override = request.getOverride();
       LOG.info("{} assigns, override={}", master.getClientIdAuditPrefix(), override);
       for (HBaseProtos.RegionSpecifier rs: request.getRegionList()) {
+        long pid = Procedure.NO_PROC_ID;
         RegionInfo ri = getRegionInfo(rs);
         if (ri == null) {
           LOG.info("Unknown={}", rs);
-          responseBuilder.addPid(Procedure.NO_PROC_ID);
+        } else {
-          continue;
+          Procedure p = this.master.getAssignmentManager().createOneAssignProcedure(ri, override);
+          if (p != null) {
+            pid = this.master.getMasterProcedureExecutor().submitProcedure(p);
           }
-        responseBuilder.addPid(this.master.getMasterProcedureExecutor().submitProcedure(this.master
+        }
-            .getAssignmentManager().createOneAssignProcedure(ri, override)));
+        responseBuilder.addPid(pid);
       }
       return responseBuilder.build();
     } catch (IOException ioe) {
@@ -2591,30 +2602,31 @@ public class MasterRpcServices extends RSRpcServices implements
   }
 
   /**
-   * A 'raw' version of unassign that does bulk and skirts Master state checks (unassigns can be
+   * A 'raw' version of unassign that does bulk and can skirt Master state checks if override
-   * made during Master startup). For use by Hbck2.
+   * is set; i.e. unassigns can be forced during Master startup or if RegionState is unclean.
+   * Used by HBCK2.
    */
   @Override
   public MasterProtos.UnassignsResponse unassigns(RpcController controller,
-      MasterProtos.UnassignsRequest request)
+      MasterProtos.UnassignsRequest request) throws ServiceException {
-      throws ServiceException {
+    checkMasterProcedureExecutor();
-    if (this.master.getMasterProcedureExecutor() == null) {
-      throw new ServiceException("Master's ProcedureExecutor not initialized; retry later");
-    }
     MasterProtos.UnassignsResponse.Builder responseBuilder =
         MasterProtos.UnassignsResponse.newBuilder();
     try {
       boolean override = request.getOverride();
       LOG.info("{} unassigns, override={}", master.getClientIdAuditPrefix(), override);
       for (HBaseProtos.RegionSpecifier rs: request.getRegionList()) {
+        long pid = Procedure.NO_PROC_ID;
         RegionInfo ri = getRegionInfo(rs);
         if (ri == null) {
           LOG.info("Unknown={}", rs);
-          responseBuilder.addPid(Procedure.NO_PROC_ID);
+        } else {
-          continue;
+          Procedure p = this.master.getAssignmentManager().createOneUnassignProcedure(ri, override);
+          if (p != null) {
+            pid = this.master.getMasterProcedureExecutor().submitProcedure(p);
           }
-        responseBuilder.addPid(this.master.getMasterProcedureExecutor().submitProcedure(this.master
+        }
-            .getAssignmentManager().createOneUnassignProcedure(ri, override)));
+        responseBuilder.addPid(pid);
       }
       return responseBuilder.build();
     } catch (IOException ioe) {

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

@@ -32,7 +32,6 @@ import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
-
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.HBaseIOException;
@@ -83,9 +82,7 @@ import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.zookeeper.KeeperException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
-
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition.TransitionCode;
@@ -580,7 +577,8 @@ public class AssignmentManager {
   private void preTransitCheck(RegionStateNode regionNode, RegionState.State[] expectedStates)
       throws HBaseIOException {
     if (regionNode.getProcedure() != null) {
-      throw new HBaseIOException(regionNode + " is currently in transition");
+      throw new HBaseIOException(regionNode + " is currently in transition; pid=" +
+        regionNode.getProcedure().getProcId());
     }
     if (!regionNode.isInState(expectedStates)) {
       throw new DoNotRetryRegionException(UNEXPECTED_STATE_REGION + regionNode);
@@ -590,25 +588,53 @@ public class AssignmentManager {
     }
   }
 
-  private TransitRegionStateProcedure createAssignProcedure(RegionInfo regionInfo, ServerName sn)
+  /**
-    throws IOException {
+   * Create an assign TransitRegionStateProcedure. Makes sure of RegionState.
-     // TODO: should we use getRegionStateNode?
+   * Throws exception if not appropriate UNLESS override is set. Used by hbck2 but also by
+   * straightline {@link #assign(RegionInfo, ServerName)} and
+   * {@link #assignAsync(RegionInfo, ServerName)}.
+   * @see #createAssignProcedure(RegionStateNode, ServerName) for a version that does NO checking
+   *   used when only when no checking needed.
+   * @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 {
     RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(regionInfo);
-    TransitRegionStateProcedure proc;
     regionNode.lock();
     try {
+      if (override) {
+        if (regionNode.getProcedure() != null) {
+          regionNode.unsetProcedure(regionNode.getProcedure());
+        }
+      } else {
         preTransitCheck(regionNode, STATES_EXPECTED_ON_ASSIGN);
-      proc = TransitRegionStateProcedure.assign(getProcedureEnvironment(), regionInfo, sn);
+      }
-      regionNode.setProcedure(proc);
+      assert regionNode.getProcedure() == null;
+      return regionNode.setProcedure(TransitRegionStateProcedure.assign(getProcedureEnvironment(),
+        regionInfo, sn));
+    } finally {
+      regionNode.unlock();
+    }
+  }
+
+  /**
+   * Create an assign TransitRegionStateProcedure. Does NO checking of RegionState.
+   * Presumes appriopriate state ripe for assign.
+   * @see #createAssignProcedure(RegionInfo, ServerName, boolean)
+   */
+  private TransitRegionStateProcedure createAssignProcedure(RegionStateNode regionNode,
+      ServerName targetServer) {
+    regionNode.lock();
+    try {
+      return regionNode.setProcedure(TransitRegionStateProcedure.assign(getProcedureEnvironment(),
+        regionNode.getRegionInfo(), targetServer));
     } finally {
       regionNode.unlock();
     }
-    return proc;
   }
 
-  // TODO: Need an async version of this for hbck2.
   public long assign(RegionInfo regionInfo, ServerName sn) throws IOException {
-    TransitRegionStateProcedure proc = createAssignProcedure(regionInfo, sn);
+    TransitRegionStateProcedure proc = createAssignProcedure(regionInfo, sn, false);
     ProcedureSyncWait.submitAndWaitProcedure(master.getMasterProcedureExecutor(), proc);
     return proc.getProcId();
   }
@@ -621,19 +647,15 @@ public class AssignmentManager {
    * Submits a procedure that assigns a region to a target server without waiting for it to finish
    * @param regionInfo the region we would like to assign
    * @param sn target server name
-   * @return
-   * @throws IOException
    */
   public Future<byte[]> assignAsync(RegionInfo regionInfo, ServerName sn) throws IOException {
-    TransitRegionStateProcedure proc = createAssignProcedure(regionInfo, sn);
+    return ProcedureSyncWait.submitProcedure(master.getMasterProcedureExecutor(),
-    return ProcedureSyncWait.submitProcedure(master.getMasterProcedureExecutor(), proc);
+      createAssignProcedure(regionInfo, sn, false));
   }
 
   /**
    * Submits a procedure that assigns a region without waiting for it to finish
    * @param regionInfo the region we would like to assign
-   * @return
-   * @throws IOException
    */
   public Future<byte[]> assignAsync(RegionInfo regionInfo) throws IOException {
     return assignAsync(regionInfo, null);
@@ -756,84 +778,86 @@ public class AssignmentManager {
     return RegionInfo.COMPARATOR.compare(left.getRegion(), right.getRegion());
   }
 
-  private TransitRegionStateProcedure createAssignProcedure(RegionStateNode regionNode,
-      ServerName targetServer, boolean override) {
-    TransitRegionStateProcedure proc;
-    regionNode.lock();
-    try {
-      if(override && regionNode.getProcedure() != null) {
-        regionNode.unsetProcedure(regionNode.getProcedure());
-      }
-      assert regionNode.getProcedure() == null;
-      proc = TransitRegionStateProcedure.assign(getProcedureEnvironment(),
-        regionNode.getRegionInfo(), targetServer);
-      regionNode.setProcedure(proc);
-    } finally {
-      regionNode.unlock();
-    }
-    return proc;
-  }
-
-  private TransitRegionStateProcedure createUnassignProcedure(RegionStateNode regionNode,
-      boolean override) {
-    TransitRegionStateProcedure proc;
-    regionNode.lock();
-    try {
-      if(override && regionNode.getProcedure() != null) {
-        regionNode.unsetProcedure(regionNode.getProcedure());
-      }
-      assert regionNode.getProcedure() == null;
-      proc = TransitRegionStateProcedure.unassign(getProcedureEnvironment(),
-          regionNode.getRegionInfo());
-      regionNode.setProcedure(proc);
-    } finally {
-      regionNode.unlock();
-    }
-    return proc;
-  }
-
   /**
    * Create one TransitRegionStateProcedure to assign a region w/o specifying a target server.
-   * This method is specified for HBCK2
+   * This method is called from HBCK2.
+   * @return an assign or null
    */
-  public TransitRegionStateProcedure createOneAssignProcedure(RegionInfo hri, boolean override) {
+  public TransitRegionStateProcedure createOneAssignProcedure(RegionInfo ri, boolean override) {
-    RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(hri);
+    TransitRegionStateProcedure trsp = null;
-    return createAssignProcedure(regionNode, null, override);
+    try {
+      trsp = createAssignProcedure(ri, null, override);
+    } catch (IOException ioe) {
+      LOG.info("Failed {} assign, override={}" +
+        (override? "": "; set override to by-pass state checks."),
+        ri.getEncodedName(), override, ioe);
+    }
+    return trsp;
   }
 
   /**
    * Create one TransitRegionStateProcedure to unassign a region.
-   * This method is specified for HBCK2
+   * This method is called from HBCK2.
+   * @return an unassign or null
    */
-  public TransitRegionStateProcedure createOneUnassignProcedure(RegionInfo hri, boolean override) {
+  public TransitRegionStateProcedure createOneUnassignProcedure(RegionInfo ri, boolean override) {
-    RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(hri);
+    RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(ri);
-    return createUnassignProcedure(regionNode, override);
+    TransitRegionStateProcedure trsp = null;
+    regionNode.lock();
+    try {
+      if (override) {
+        if (regionNode.getProcedure() != null) {
+          regionNode.unsetProcedure(regionNode.getProcedure());
+        }
+      } else {
+        // This is where we could throw an exception; i.e. override is false.
+        preTransitCheck(regionNode, STATES_EXPECTED_ON_UNASSIGN_OR_MOVE);
+      }
+      assert regionNode.getProcedure() == null;
+      trsp = TransitRegionStateProcedure.unassign(getProcedureEnvironment(),
+        regionNode.getRegionInfo());
+      regionNode.setProcedure(trsp);
+    } catch (IOException ioe) {
+      // 'override' must be false here.
+      LOG.info("Failed {} unassign, override=false; set override to by-pass state checks.",
+        ri.getEncodedName(), ioe);
+    } finally{
+      regionNode.unlock();
+    }
+    return trsp;
   }
 
   /**
    * Create an array of TransitRegionStateProcedure w/o specifying a target server.
+   * Used as fallback of caller is unable to do {@link #createAssignProcedures(Map)}.
    * <p/>
    * If no target server, at assign time, we will try to use the former location of the region if
    * one exists. This is how we 'retain' the old location across a server restart.
    * <p/>
    * Should only be called when you can make sure that no one can touch these regions other than
-   * you. For example, when you are creating table.
+   * you. For example, when you are creating or enabling table. Presumes all Regions are in
+   * appropriate state ripe for assign; no checking of Region state is done in here.
+   * @see #createAssignProcedures(Map)
    */
   public TransitRegionStateProcedure[] createAssignProcedures(List<RegionInfo> hris) {
     return hris.stream().map(hri -> regionStates.getOrCreateRegionStateNode(hri))
-        .map(regionNode -> createAssignProcedure(regionNode, null, false))
+        .map(regionNode -> createAssignProcedure(regionNode, null))
         .sorted(AssignmentManager::compare).toArray(TransitRegionStateProcedure[]::new);
   }
 
   /**
+   * Tied to {@link #createAssignProcedures(List)} in that it is called if caller is unable to run
+   * this method. Presumes all Regions are in appropriate state ripe for assign; no checking
+   * of Region state is done in here.
    * @param assignments Map of assignments from which we produce an array of AssignProcedures.
    * @return Assignments made from the passed in <code>assignments</code>
+   * @see #createAssignProcedures(List)
    */
   private TransitRegionStateProcedure[] createAssignProcedures(
       Map<ServerName, List<RegionInfo>> assignments) {
     return assignments.entrySet().stream()
       .flatMap(e -> e.getValue().stream().map(hri -> regionStates.getOrCreateRegionStateNode(hri))
-        .map(regionNode -> createAssignProcedure(regionNode, e.getKey(), false)))
+        .map(regionNode -> createAssignProcedure(regionNode, e.getKey())))
       .sorted(AssignmentManager::compare).toArray(TransitRegionStateProcedure[]::new);
   }
 
@@ -1054,8 +1078,8 @@ public class AssignmentManager {
 
     // If the RS is < 2.0 throw an exception to abort the operation, we are handling the split
     if (master.getServerManager().getVersionNumber(serverName) < 0x0200000) {
-      throw new UnsupportedOperationException(String.format(
+      throw new UnsupportedOperationException(String.format("Split handled by the master: " +
-        "Split handled by the master: parent=%s hriA=%s hriB=%s", parent.getShortNameToLog(), hriA, hriB));
+        "parent=%s hriA=%s hriB=%s", parent.getShortNameToLog(), hriA, hriB));
     }
   }
 
@@ -1346,9 +1370,13 @@ public class AssignmentManager {
 
     public boolean isRegionTwiceOverThreshold(final RegionInfo regionInfo) {
       Map<String, RegionState> m = this.ritsOverThreshold;
-      if (m == null) return false;
+      if (m == null) {
+        return false;
+      }
       final RegionState state = m.get(regionInfo.getEncodedName());
-      if (state == null) return false;
+      if (state == null) {
+        return false;
+      }
       return (statTimestamp - state.getStamp()) > (ritThreshold * 2);
     }
 
@@ -1638,13 +1666,12 @@ public class AssignmentManager {
   // ============================================================================================
   /**
    * Used by the client (via master) to identify if all regions have the schema updates
-   *
-   * @param tableName
    * @return Pair indicating the status of the alter command (pending/total)
-   * @throws IOException
    */
   public Pair<Integer, Integer> getReopenStatus(TableName tableName) {
-    if (isTableDisabled(tableName)) return new Pair<Integer, Integer>(0, 0);
+    if (isTableDisabled(tableName)) {
+      return new Pair<Integer, Integer>(0, 0);
+    }
 
     final List<RegionState> states = regionStates.getTableRegionStates(tableName);
     int ritCount = 0;
@@ -1973,7 +2000,9 @@ public class AssignmentManager {
         assignQueueFullCond.await();
       }
 
-      if (!isRunning()) return null;
+      if (!isRunning()) {
+        return null;
+      }
       assignQueueFullCond.await(assignDispatchWaitMillis, TimeUnit.MILLISECONDS);
       regions = new HashMap<RegionInfo, RegionStateNode>(pendingAssignQueue.size());
       for (RegionStateNode regionNode: pendingAssignQueue) {
@@ -2110,7 +2139,9 @@ public class AssignmentManager {
       throw new HBaseIOException("unable to compute plans for regions=" + regions.size());
     }
 
-    if (plan.isEmpty()) return;
+    if (plan.isEmpty()) {
+      return;
+    }
 
     int evcount = 0;
     for (Map.Entry<ServerName, List<RegionInfo>> entry: plan.entrySet()) {

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

@@ -190,10 +190,11 @@ public class RegionStateNode implements Comparable<RegionStateNode> {
     return lastRegionLocation;
   }
 
-  public void setProcedure(TransitRegionStateProcedure proc) {
+  public TransitRegionStateProcedure setProcedure(TransitRegionStateProcedure proc) {
     assert this.procedure == null;
     this.procedure = proc;
     ritMap.put(regionInfo, this);
+    return proc;
   }
 
   public void unsetProcedure(TransitRegionStateProcedure proc) {

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

@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -18,8 +18,8 @@
 package org.apache.hadoop.hbase.client;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
-
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -28,7 +28,6 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.CountDownLatch;
 import java.util.stream.Collectors;
-
 import org.apache.hadoop.hbase.Coprocessor;
 import org.apache.hadoop.hbase.CoprocessorEnvironment;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -67,9 +66,7 @@ import org.junit.runners.Parameterized.Parameter;
 import org.junit.runners.Parameterized.Parameters;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
-
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
 
 /**
@@ -88,7 +85,7 @@ public class TestHbck {
   @Rule
   public TestName name = new TestName();
 
-  @Parameter
+  @SuppressWarnings("checkstyle:VisibilityModifier") @Parameter
   public boolean async;
 
   private static final TableName TABLE_NAME = TableName.valueOf(TestHbck.class.getSimpleName());
@@ -229,6 +226,26 @@ public class TestHbck {
       List<Long> pids =
         hbck.unassigns(regions.stream().map(r -> r.getEncodedName()).collect(Collectors.toList()));
       waitOnPids(pids);
+      // Rerun the unassign. 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(r -> r.getEncodedName()).collect(Collectors.toList()));
+      waitOnPids(pids);
+      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
+      // 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(r -> r.getEncodedName()).collect(Collectors.toList()),
+          true);
+      waitOnPids(pids);
+      for (long pid: pids) {
+        assertNotEquals(Procedure.NO_PROC_ID, pid);
+      }
+      // Clean-up by bypassing all the unassigns we just made so tests can continue.
+      hbck.bypassProcedure(pids, 10000, true, true);
       for (RegionInfo ri : regions) {
         RegionState rs = TEST_UTIL.getHBaseCluster().getMaster().getAssignmentManager()
           .getRegionStates().getRegionState(ri.getEncodedName());
@@ -238,6 +255,13 @@ public class TestHbck {
       pids =
         hbck.assigns(regions.stream().map(r -> r.getEncodedName()).collect(Collectors.toList()));
       waitOnPids(pids);
+      // Rerun the assign. Should fail for all Regions since they already assigned; failed
+      // assign will manifest as all pids being -1 (ever since HBASE-24885).
+      pids =
+        hbck.assigns(regions.stream().map(r -> r.getEncodedName()).collect(Collectors.toList()));
+      for (long pid: pids) {
+        assertEquals(Procedure.NO_PROC_ID, pid);
+      }
       for (RegionInfo ri : regions) {
         RegionState rs = TEST_UTIL.getHBaseCluster().getMaster().getAssignmentManager()
           .getRegionStates().getRegionState(ri.getEncodedName());
@@ -248,7 +272,7 @@ public class TestHbck {
       pids = hbck.assigns(
         Arrays.stream(new String[] { "a", "some rubbish name" }).collect(Collectors.toList()));
       for (long pid : pids) {
-        assertEquals(org.apache.hadoop.hbase.procedure2.Procedure.NO_PROC_ID, pid);
+        assertEquals(Procedure.NO_PROC_ID, pid);
       }
     }
   }
@@ -288,7 +312,7 @@ public class TestHbck {
 
   public static class FailingSplitAfterMetaUpdatedMasterObserver
       implements MasterCoprocessor, MasterObserver {
-    public volatile CountDownLatch latch;
+    @SuppressWarnings("checkstyle:VisibilityModifier") public volatile CountDownLatch latch;
 
     @Override
     public void start(CoprocessorEnvironment e) throws IOException {
@@ -315,7 +339,7 @@ public class TestHbck {
 
   public static class FailingMergeAfterMetaUpdatedMasterObserver
       implements MasterCoprocessor, MasterObserver {
-    public volatile CountDownLatch latch;
+    @SuppressWarnings("checkstyle:VisibilityModifier") public volatile CountDownLatch latch;
 
     @Override
     public void start(CoprocessorEnvironment e) throws IOException {