HBASE-19980 NullPointerException when restoring a snapshot after splitting a region

Signed-off-by: tedyu <yuzhihong@gmail.com>

hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/RestoreSnapshotHelper.java

@@ -195,11 +195,33 @@ public class RestoreSnapshotHelper {
     // this instance, by removing the regions already present in the restore dir.
     Set<String> regionNames = new HashSet<>(regionManifests.keySet());
 
+    List<RegionInfo> tableRegions = getTableRegions();
+
     RegionInfo mobRegion = MobUtils.getMobRegionInfo(snapshotManifest.getTableDescriptor()
         .getTableName());
+    if (tableRegions != null) {
+      // restore the mob region in case
+      if (regionNames.contains(mobRegion.getEncodedName())) {
+        monitor.rethrowException();
+        status.setStatus("Restoring mob region...");
+        List<RegionInfo> mobRegions = new ArrayList<>(1);
+        mobRegions.add(mobRegion);
+        restoreHdfsMobRegions(exec, regionManifests, mobRegions);
+        regionNames.remove(mobRegion.getEncodedName());
+        status.setStatus("Finished restoring mob region.");
+      }
+    }
+    if (regionNames.contains(mobRegion.getEncodedName())) {
+      // add the mob region
+      monitor.rethrowException();
+      status.setStatus("Cloning mob region...");
+      cloneHdfsMobRegion(regionManifests, mobRegion);
+      regionNames.remove(mobRegion.getEncodedName());
+      status.setStatus("Finished cloning mob region.");
+    }
+
     // Identify which region are still available and which not.
     // NOTE: we rely upon the region name as: "table name, start key, end key"
-    List<RegionInfo> tableRegions = getTableRegions();
     if (tableRegions != null) {
       monitor.rethrowException();
       for (RegionInfo regionInfo: tableRegions) {
@@ -213,50 +235,40 @@ public class RestoreSnapshotHelper {
           metaChanges.addRegionToRemove(regionInfo);
         }
       }
-
-      // Restore regions using the snapshot data
-      monitor.rethrowException();
-      status.setStatus("Restoring table regions...");
-      if (regionNames.contains(mobRegion.getEncodedName())) {
-        // restore the mob region in case
-        List<RegionInfo> mobRegions = new ArrayList<>(1);
-        mobRegions.add(mobRegion);
-        restoreHdfsMobRegions(exec, regionManifests, mobRegions);
-        regionNames.remove(mobRegion.getEncodedName());
-      }
-      restoreHdfsRegions(exec, regionManifests, metaChanges.getRegionsToRestore());
-      status.setStatus("Finished restoring all table regions.");
-
-      // Remove regions from the current table
-      monitor.rethrowException();
-      status.setStatus("Starting to delete excess regions from table");
-      removeHdfsRegions(exec, metaChanges.getRegionsToRemove());
-      status.setStatus("Finished deleting excess regions from table.");
     }
 
     // Regions to Add: present in the snapshot but not in the current table
+    List<RegionInfo> regionsToAdd = new ArrayList<>(regionNames.size());
     if (regionNames.size() > 0) {
-      List<RegionInfo> regionsToAdd = new ArrayList<>(regionNames.size());
-
       monitor.rethrowException();
-      // add the mob region
-      if (regionNames.contains(mobRegion.getEncodedName())) {
-        cloneHdfsMobRegion(regionManifests, mobRegion);
-        regionNames.remove(mobRegion.getEncodedName());
-      }
       for (String regionName: regionNames) {
         LOG.info("region to add: " + regionName);
-        regionsToAdd.add(ProtobufUtil.toRegionInfo(regionManifests.get(regionName).getRegionInfo()));
+        regionsToAdd.add(ProtobufUtil.toRegionInfo(regionManifests.get(regionName)
+            .getRegionInfo()));
       }
-
-      // Create new regions cloning from the snapshot
-      monitor.rethrowException();
-      status.setStatus("Cloning regions...");
-      RegionInfo[] clonedRegions = cloneHdfsRegions(exec, regionManifests, regionsToAdd);
-      metaChanges.setNewRegions(clonedRegions);
-      status.setStatus("Finished cloning regions.");
     }
 
+    // Create new regions cloning from the snapshot
+    // HBASE-19980: We need to call cloneHdfsRegions() before restoreHdfsRegions() because
+    // regionsMap is constructed in cloneHdfsRegions() and it can be used in restoreHdfsRegions().
+    monitor.rethrowException();
+    status.setStatus("Cloning regions...");
+    RegionInfo[] clonedRegions = cloneHdfsRegions(exec, regionManifests, regionsToAdd);
+    metaChanges.setNewRegions(clonedRegions);
+    status.setStatus("Finished cloning regions.");
+
+    // Restore regions using the snapshot data
+    monitor.rethrowException();
+    status.setStatus("Restoring table regions...");
+    restoreHdfsRegions(exec, regionManifests, metaChanges.getRegionsToRestore());
+    status.setStatus("Finished restoring all table regions.");
+
+    // Remove regions from the current table
+    monitor.rethrowException();
+    status.setStatus("Starting to delete excess regions from table");
+    removeHdfsRegions(exec, metaChanges.getRegionsToRemove());
+    status.setStatus("Finished deleting excess regions from table.");
+
     LOG.info("finishing restore table regions using snapshot=" + snapshotDesc);
 
     return metaChanges;
@@ -742,11 +754,16 @@ public class RestoreSnapshotHelper {
 
     // Add the daughter region to the map
     String regionName = Bytes.toString(regionsMap.get(regionInfo.getEncodedNameAsBytes()));
+    if (regionName == null) {
+      regionName = regionInfo.getEncodedName();
+    }
     LOG.debug("Restore reference " + regionName + " to " + clonedRegionName);
     synchronized (parentsMap) {
       Pair<String, String> daughters = parentsMap.get(clonedRegionName);
       if (daughters == null) {
-        daughters = new Pair<>(regionName, null);
+        // In case one side of the split is already compacted, regionName is put as both first and
+        // second of Pair
+        daughters = new Pair<>(regionName, regionName);
         parentsMap.put(clonedRegionName, daughters);
       } else if (!regionName.equals(daughters.getFirst())) {
         daughters.setSecond(regionName);

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

@@ -23,6 +23,7 @@ import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 import org.apache.hadoop.conf.Configuration;
@@ -295,6 +296,25 @@ public class TestRestoreSnapshotFromClient {
     }
   }
 
+  @Test
+  public void testRestoreSnapshotAfterSplittingRegions() throws IOException, InterruptedException {
+    List<RegionInfo> regionInfos = admin.getRegions(tableName);
+    RegionReplicaUtil.removeNonDefaultRegions(regionInfos);
+
+    // Split the first region
+    splitRegion(regionInfos.get(0));
+
+    // Take a snapshot
+    admin.snapshot(snapshotName1, tableName);
+
+    // Restore the snapshot
+    admin.disableTable(tableName);
+    admin.restoreSnapshot(snapshotName1);
+    admin.enableTable(tableName);
+
+    verifyRowCount(TEST_UTIL, tableName, snapshot1Rows);
+  }
+
   // ==========================================================================
   //  Helpers
   // ==========================================================================