From 4a80b19d7f780652e3b1c9e8c80e60c4dfed8bb5 Mon Sep 17 00:00:00 2001
From: Ankit Singhal <ankitsinghal59@gmail.com>
Date: Mon, 2 Jul 2018 16:07:34 +0800
Subject: [PATCH] HBASE-20817 Infinite loop when executing
 ReopenTableRegionsProcedure

Signed-off-by: zhangduo <zhangduo@apache.org>
---
 .../assignment/MoveRegionProcedure.java       |  8 +++---
 .../hadoop/hbase/regionserver/HRegion.java    | 23 +++++++++-------
 .../hadoop/hbase/client/TestAdmin1.java       | 27 ++++++++++++++++---
 3 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java
index 139d41dcc96..6135ce1cc61 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java
@@ -66,8 +66,10 @@ public class MoveRegionProcedure extends AbstractStateMachineRegionProcedure<Mov
       throws HBaseIOException {
     super(env, plan.getRegionInfo());
     this.plan = plan;
-    preflightChecks(env, true);
-    checkOnline(env, plan.getRegionInfo());
+    if (check) {
+      preflightChecks(env, true);
+      checkOnline(env, plan.getRegionInfo());
+    }
   }
 
   @Override
@@ -135,7 +137,7 @@ public class MoveRegionProcedure extends AbstractStateMachineRegionProcedure<Mov
 
   @Override
   protected MoveRegionState getState(final int stateId) {
-    return MoveRegionState.valueOf(stateId);
+    return MoveRegionState.forNumber(stateId);
   }
 
   @Override
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
index a628437deed..762015b10f1 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
@@ -967,7 +967,11 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
     long maxSeqIdFromFile =
       WALSplitter.getMaxRegionSequenceId(fs.getFileSystem(), fs.getRegionDir());
     long nextSeqId = Math.max(maxSeqId, maxSeqIdFromFile) + 1;
-    if (writestate.writesEnabled) {
+    // The openSeqNum will always be increase even for read only region, as we rely on it to
+    // determine whether a region has been successfully reopend, so here we always need to update
+    // the max sequence id file.
+    if (RegionReplicaUtil.isDefaultReplica(getRegionInfo())) {
+      LOG.debug("writing seq id for {}", this.getRegionInfo().getEncodedName());
       WALSplitter.writeRegionSequenceIdFile(fs.getFileSystem(), fs.getRegionDir(), nextSeqId - 1);
     }
 
@@ -1643,7 +1647,10 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
       }
 
       status.setStatus("Writing region close event to WAL");
-      if (!abort && wal != null && getRegionServerServices() != null && !writestate.readOnly) {
+      // Always write close marker to wal even for read only table. This is not a big problem as we
+      // do not write any data into the region.
+      if (!abort && wal != null && getRegionServerServices() != null &&
+        RegionReplicaUtil.isDefaultReplica(getRegionInfo())) {
         writeRegionCloseMarker(wal);
       }
 
@@ -7010,7 +7017,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
    * HRegion#getMinSequenceId() to ensure the wal id is properly kept
    * up.  HRegionStore does this every time it opens a new region.
    * @return new HRegion
-   * @throws IOException
    */
   public static HRegion openHRegion(final Configuration conf, final FileSystem fs,
       final Path rootDir, final RegionInfo info, final TableDescriptor htd, final WAL wal)
@@ -7032,7 +7038,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
    * @param rsServices An interface we can request flushes against.
    * @param reporter An interface we can report progress against.
    * @return new HRegion
-   * @throws IOException
    */
   public static HRegion openHRegion(final Configuration conf, final FileSystem fs,
       final Path rootDir, final RegionInfo info, final TableDescriptor htd, final WAL wal,
@@ -7056,7 +7061,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
    * @param rsServices An interface we can request flushes against.
    * @param reporter An interface we can report progress against.
    * @return new HRegion
-   * @throws IOException
    */
   public static HRegion openHRegion(final Configuration conf, final FileSystem fs,
       final Path rootDir, final Path tableDir, final RegionInfo info, final TableDescriptor htd,
@@ -7081,7 +7085,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
    * @param other original object
    * @param reporter An interface we can report progress against.
    * @return new HRegion
-   * @throws IOException
    */
   public static HRegion openHRegion(final HRegion other, final CancelableProgressable reporter)
       throws IOException {
@@ -7112,8 +7115,11 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
     checkClassLoading();
     this.openSeqNum = initialize(reporter);
     this.mvcc.advanceTo(openSeqNum);
-    if (wal != null && getRegionServerServices() != null && !writestate.readOnly) {
-      // Only write the region open event marker to WAL if we are not read-only.
+    // The openSeqNum must be increased every time when a region is assigned, as we rely on it to
+    // determine whether a region has been successfully reopened. So here we always write open
+    // marker, even if the table is read only.
+    if (wal != null && getRegionServerServices() != null &&
+      RegionReplicaUtil.isDefaultReplica(getRegionInfo())) {
       writeRegionOpenMarker(wal, openSeqNum);
     }
     return this;
@@ -7126,7 +7132,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
    * @param info Info for region to be opened.
    * @param htd the table descriptor
    * @return new HRegion
-   * @throws IOException e
    */
   public static HRegion openReadOnlyFileSystemHRegion(final Configuration conf, final FileSystem fs,
       final Path tableDir, RegionInfo info, final TableDescriptor htd) throws IOException {
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java
index e0eef20b185..b3df2cc1432 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java
@@ -31,7 +31,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
-
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HBaseTestingUtility;
 import org.apache.hadoop.hbase.HColumnDescriptor;
@@ -69,6 +68,7 @@ import org.junit.experimental.categories.Category;
 import org.junit.rules.TestName;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+
 import org.apache.hadoop.hbase.shaded.protobuf.RequestConverter;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.MergeTableRegionsRequest;
 
@@ -500,10 +500,31 @@ public class TestAdmin1 {
        }
    }
 
+  /**
+   * Verify schema change for read only table
+   */
+  @Test
+  public void testReadOnlyTableModify() throws IOException, InterruptedException {
+    final TableName tableName = TableName.valueOf(name.getMethodName());
+    TEST_UTIL.createTable(tableName, HConstants.CATALOG_FAMILY).close();
+
+    // Make table read only
+    TableDescriptor htd = TableDescriptorBuilder.newBuilder(this.admin.getDescriptor(tableName))
+      .setReadOnly(true).build();
+    admin.modifyTable(htd);
+
+    // try to modify the read only table now
+    htd = TableDescriptorBuilder.newBuilder(this.admin.getDescriptor(tableName))
+      .setCompactionEnabled(false).build();
+    admin.modifyTable(htd);
+    // Delete the table
+    this.admin.disableTable(tableName);
+    this.admin.deleteTable(tableName);
+    assertFalse(this.admin.tableExists(tableName));
+  }
+
   /**
    * Verify schema modification takes.
-   * @throws IOException
-   * @throws InterruptedException
    */
   @Test
   public void testOnlineChangeTableSchema() throws IOException, InterruptedException {