HBASE-12924 HRegionServer#MovedRegionsCleaner Chore does not start

Signed-off-by: Elliott Clark <eclark@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java

@@ -787,7 +787,7 @@ public class HRegionServer extends HasThread implements
     this.leases = new Leases(this.threadWakeFrequency);
 
     // Create the thread to clean the moved regions list
-    movedRegionsCleaner = MovedRegionsCleaner.createAndStart(this);
+    movedRegionsCleaner = MovedRegionsCleaner.create(this);
 
     if (this.nonceManager != null) {
       // Create the scheduled chore that cleans up nonces.
@@ -1636,6 +1636,7 @@ public class HRegionServer extends HasThread implements
     if (this.healthCheckChore != null) choreService.scheduleChore(healthCheckChore);
     if (this.nonceManagerChore != null) choreService.scheduleChore(nonceManagerChore);
     if (this.storefileRefresher != null) choreService.scheduleChore(storefileRefresher);
+    if (this.movedRegionsCleaner != null) choreService.scheduleChore(movedRegionsCleaner);
 
     // Leases is not a Thread. Internally it runs a daemon thread. If it gets
     // an unhandled exception, it will just exit.
@@ -1981,6 +1982,7 @@ public class HRegionServer extends HasThread implements
     if (this.periodicFlusher != null) periodicFlusher.cancel(true);
     if (this.healthCheckChore != null) healthCheckChore.cancel(true);
     if (this.storefileRefresher != null) storefileRefresher.cancel(true);
+    if (this.movedRegionsCleaner != null) movedRegionsCleaner.cancel(true);
 
     if (this.cacheFlusher != null) {
       this.cacheFlusher.join();
@@ -2912,21 +2914,31 @@ public class HRegionServer extends HasThread implements
     }
   }
 
+  /*
+   * Use this to allow tests to override and schedule more frequently.
+   */
+
+  protected int movedRegionCleanerPeriod() {
+        return TIMEOUT_REGION_MOVED;
+  }
+
   /**
    * Creates a Chore thread to clean the moved region cache.
    */
-  protected static class MovedRegionsCleaner extends ScheduledChore implements Stoppable {
+
+  protected final static class MovedRegionsCleaner extends ScheduledChore implements Stoppable {
     private HRegionServer regionServer;
     Stoppable stoppable;
 
     private MovedRegionsCleaner(
       HRegionServer regionServer, Stoppable stoppable){
-      super("MovedRegionsCleaner for region " + regionServer, stoppable, TIMEOUT_REGION_MOVED);
+      super("MovedRegionsCleaner for region " + regionServer, stoppable,
+          regionServer.movedRegionCleanerPeriod());
       this.regionServer = regionServer;
       this.stoppable = stoppable;
     }
 
-    static MovedRegionsCleaner createAndStart(HRegionServer rs){
+    static MovedRegionsCleaner create(HRegionServer rs){
       Stoppable stoppable = new Stoppable() {
         private volatile boolean isStopped = false;
         @Override public void stop(String why) { isStopped = true;}

hbase-server/src/test/java/org/apache/hadoop/hbase/TestMovedRegionsCleaner.java

@@ -0,0 +1,95 @@
+/**
+ *
+ * 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
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+
+
+/**
+ * Test whether background cleanup of MovedRegion entries is happening
+ */
+@Category( MediumTests.class ) public class TestMovedRegionsCleaner {
+
+  public static final Log LOG = LogFactory.getLog(TestRegionRebalancing.class);
+  private final HBaseTestingUtility UTIL = new HBaseTestingUtility();
+
+  public static int numCalls = 0;
+
+  private static class TestMockRegionServer extends MiniHBaseCluster.MiniHBaseClusterRegionServer {
+
+    public TestMockRegionServer(Configuration conf, CoordinatedStateManager cp)
+        throws IOException, InterruptedException {
+      super(conf, cp);
+    }
+
+    protected int movedRegionCleanerPeriod() {
+      return 500;
+    }
+
+    @Override protected void cleanMovedRegions() {
+      // count the number of calls that are being made to this
+      //
+      numCalls++;
+      super.cleanMovedRegions();
+    }
+  }
+
+  @After public void after() throws Exception {
+    UTIL.shutdownMiniCluster();
+  }
+
+  @Before public void before() throws Exception {
+    UTIL.getConfiguration()
+        .setStrings(HConstants.REGION_SERVER_IMPL, TestMockRegionServer.class.getName());
+    UTIL.startMiniCluster(1);
+  }
+
+  /**
+   * Start the cluster, wait for some time and verify that the background
+   * MovedRegion cleaner indeed gets called
+   *
+   * @throws IOException
+   * @throws InterruptedException
+   */
+  @Test public void testMovedRegionsCleaner() throws IOException, InterruptedException {
+    // We need to sleep long enough to trigger at least one round of background calls
+    // to MovedRegionCleaner happen. Currently the period is set to 500ms.
+    // Setting the sleep here for 2s just to be safe
+    //
+    UTIL.waitFor(2000, new Waiter.Predicate<IOException>() {
+      @Override
+      public boolean evaluate() throws IOException {
+
+        // verify that there was at least one call to the cleanMovedRegions function
+        //
+        return numCalls > 0;
+      }
+    });
+  }
+}