HBASE-19703 Functionality added as part of HBASE-12583 is not working after moving the split code to master

Co-authored-by: Michael Stack <stack@apache.org>

Signed-off-by: Chia-Ping Tsai <chia7712@gmail.com>

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

@@ -32,7 +32,6 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
-
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
@@ -63,16 +62,20 @@ import org.apache.hadoop.hbase.quotas.QuotaExceededException;
 import org.apache.hadoop.hbase.regionserver.HRegionFileSystem;
 import org.apache.hadoop.hbase.regionserver.HStore;
 import org.apache.hadoop.hbase.regionserver.HStoreFile;
+import org.apache.hadoop.hbase.regionserver.RegionSplitPolicy;
 import org.apache.hadoop.hbase.regionserver.StoreFileInfo;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
 import org.apache.hadoop.hbase.util.FSUtils;
 import org.apache.hadoop.hbase.util.Pair;
 import org.apache.hadoop.hbase.util.Threads;
+import org.apache.hadoop.util.ReflectionUtils;
 import org.apache.yetus.audience.InterfaceAudience;
 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.AdminProtos.GetRegionInfoResponse;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos;
@@ -91,6 +94,8 @@ public class SplitTableRegionProcedure
   private RegionInfo daughter_1_RI;
   private RegionInfo daughter_2_RI;
   private byte[] bestSplitRow;
+  private TableDescriptor htd;
+  private RegionSplitPolicy splitPolicy;
 
   public SplitTableRegionProcedure() {
     // Required by the Procedure framework to create the procedure on replay
@@ -115,6 +120,16 @@ public class SplitTableRegionProcedure
         .setSplit(false)
         .setRegionId(rid)
         .build();
+    this.htd = env.getMasterServices().getTableDescriptors().get(getTableName());
+    if(this.htd.getRegionSplitPolicyClassName() != null) {
+      // Since we don't have region reference here, creating the split policy instance without it.
+      // This can be used to invoke methods which don't require Region reference. This instantiation
+      // of a class on Master-side though it only makes sense on the RegionServer-side is
+      // for Phoenix Local Indexing. Refer HBASE-12583 for more information.
+      Class<? extends RegionSplitPolicy> clazz =
+          RegionSplitPolicy.getSplitPolicyClass(this.htd, env.getMasterConfiguration());
+      this.splitPolicy = ReflectionUtils.newInstance(clazz, env.getMasterConfiguration());
+    }
   }
 
   /**
@@ -597,7 +612,6 @@ public class SplitTableRegionProcedure
     final List<Future<Pair<Path,Path>>> futures = new ArrayList<Future<Pair<Path,Path>>>(nbFiles);
 
     // Split each store file.
-    final TableDescriptor htd = env.getMasterServices().getTableDescriptors().get(getTableName());
     for (Map.Entry<String, Collection<StoreFileInfo>>e: files.entrySet()) {
       byte [] familyName = Bytes.toBytes(e.getKey());
       final ColumnFamilyDescriptor hcd = htd.getColumnFamily(familyName);
@@ -668,7 +682,7 @@ public class SplitTableRegionProcedure
   }
 
   private Pair<Path, Path> splitStoreFile(HRegionFileSystem regionFs, byte[] family, HStoreFile sf)
-      throws IOException {
+    throws IOException {
     if (LOG.isDebugEnabled()) {
       LOG.debug("pid=" + getProcId() + " splitting started for store file: " +
           sf.getPath() + " for region: " + getParentRegion().getShortNameToLog());
@@ -676,10 +690,10 @@ public class SplitTableRegionProcedure
 
     final byte[] splitRow = getSplitRow();
     final String familyName = Bytes.toString(family);
-    final Path path_first =
-        regionFs.splitStoreFile(this.daughter_1_RI, familyName, sf, splitRow, false, null);
-    final Path path_second =
-        regionFs.splitStoreFile(this.daughter_2_RI, familyName, sf, splitRow, true, null);
+    final Path path_first = regionFs.splitStoreFile(this.daughter_1_RI, familyName, sf, splitRow,
+        false, splitPolicy);
+    final Path path_second = regionFs.splitStoreFile(this.daughter_2_RI, familyName, sf, splitRow,
+       true, splitPolicy);
     if (LOG.isDebugEnabled()) {
       LOG.debug("pid=" + getProcId() + " splitting complete for store file: " +
           sf.getPath() + " for region: " + getParentRegion().getShortNameToLog());

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

@@ -652,7 +652,9 @@ public class HRegionFileSystem {
    * @param f File to split.
    * @param splitRow Split Row
    * @param top True if we are referring to the top half of the hfile.
-   * @param splitPolicy
+   * @param splitPolicy A split policy instance; be careful! May not be full populated; e.g. if
+   *                    this method is invoked on the Master side, then the RegionSplitPolicy will
+   *                    NOT have a reference to a Region.
    * @return Path to created reference.
    * @throws IOException
    */

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

@@ -28,12 +28,12 @@ import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.util.ReflectionUtils;
 import org.apache.yetus.audience.InterfaceAudience;
-
 import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
 
 
 /**
- * A split policy determines when a region should be split.
+ * A split policy determines when a Region should be split.
+ *
  * @see SteppingSplitPolicy Default split policy since 2.0.0
  * @see IncreasingToUpperBoundRegionSplitPolicy Default split policy since
  *      0.94.0
@@ -46,6 +46,9 @@ public abstract class RegionSplitPolicy extends Configured {
 
   /**
    * The region configured for this split policy.
+   * As of hbase-2.0.0, RegionSplitPolicy can be instantiated on the Master-side so the
+   * Phoenix local-indexer can block default hbase behavior. This is an exotic usage. Should not
+   * trouble any other users of RegionSplitPolicy.
    */
   protected HRegion region;
 
@@ -103,6 +106,7 @@ public abstract class RegionSplitPolicy extends Configured {
    */
   public static RegionSplitPolicy create(HRegion region,
       Configuration conf) throws IOException {
+    Preconditions.checkNotNull(region, "Region should not be null.");
     Class<? extends RegionSplitPolicy> clazz = getSplitPolicyClass(
         region.getTableDescriptor(), conf);
     RegionSplitPolicy policy = ReflectionUtils.newInstance(clazz, conf);
@@ -133,11 +137,17 @@ public abstract class RegionSplitPolicy extends Configured {
   /**
    * In {@link HRegionFileSystem#splitStoreFile(org.apache.hadoop.hbase.client.RegionInfo, String,
    * HStoreFile, byte[], boolean, RegionSplitPolicy)} we are not creating the split reference
-   * if split row not lies in the StoreFile range. But in some use cases we may need to create
-   * the split reference even when the split row not lies in the range. This method can be used
-   * to decide, whether to skip the the StoreFile range check or not.
-   * @return whether to skip the StoreFile range check or not
-   * @param familyName
+   * if split row does not lie inside the StoreFile range. But in some use cases we may need to
+   * create the split reference even when the split row does not lie inside the StoreFile range.
+   * This method can be used to decide, whether to skip the the StoreFile range check or not.
+   *
+   * <p>This method is not for general use. It is a mechanism put in place by Phoenix
+   * local indexing to defeat standard hbase behaviors. Phoenix local indices are very likely
+   * the only folks who would make use of this method. On the Master-side, we will instantiate
+   * a RegionSplitPolicy instance and run this method ONLY... none of the others make sense
+   * on the Master-side.</p>
+   *
+   * TODO: Shutdown this phoenix specialization or do it via some other means.
    * @return whether to skip the StoreFile range check or not
    */
   protected boolean skipStoreFileRangeCheck(String familyName) {

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java

@@ -990,7 +990,7 @@ public class TestSplitTransactionOnCluster {
     }
   }
 
-  static class CustomSplitPolicy extends RegionSplitPolicy {
+  static class CustomSplitPolicy extends IncreasingToUpperBoundRegionSplitPolicy {
 
     @Override
     protected boolean shouldSplit() {