HBASE-28354 RegionSizeCalculator throws NPE when regions are in trans…

…ition (#5699)

When a region is in transition, it may briefly have a null ServerName in meta. The
RegionSizeCalculator calls RegionLocator.getAllRegionLocations() and does not handle the
possibility that a RegionLocation.getServerName() could be null. The ServerName is eventually
passed into an Admin call, which results in an NPE.

This has come up in other contexts. For example, taking a look at getAllRegionLocations() impl, we
have checks to ensure that we don't call null server names. We need to similarly handle the
possibility of nulls in RegionSizeCalculator.

Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Hui Ruan <[email protected]>

hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/RegionSizeCalculator.java

@@ -21,8 +21,10 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.stream.Collectors;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HRegionLocation;
 import org.apache.hadoop.hbase.RegionMetrics;
@@ -35,8 +37,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import org.apache.hbase.thirdparty.com.google.common.collect.Sets;
-
 /**
  * Computes size of each region for given table and given column families. The value is used by
  * MapReduce for better scheduling.
@@ -96,12 +96,11 @@ private void init(RegionLocator regionLocator, Admin admin) throws IOException {
   }
 
   private Set<ServerName> getRegionServersOfTable(RegionLocator regionLocator) throws IOException {
-
-    Set<ServerName> tableServers = Sets.newHashSet();
-    for (HRegionLocation regionLocation : regionLocator.getAllRegionLocations()) {
-      tableServers.add(regionLocation.getServerName());
-    }
-    return tableServers;
+    // The region locations could contain `null` ServerName instances if the region is currently
+    // in transition, we filter those out for now, which impacts the size calculation for these
+    // regions temporarily until the ServerName gets filled in later
+    return regionLocator.getAllRegionLocations().stream().map(HRegionLocation::getServerName)
+      .filter(Objects::nonNull).collect(Collectors.toSet());
   }
 
   boolean enabled(Configuration configuration) {

hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestRegionSizeCalculator.java

@@ -23,6 +23,8 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -67,8 +69,9 @@ public void testSimpleTestCase() throws Exception {
     assertEquals(123 * megabyte, calculator.getRegionSize(Bytes.toBytes("region1")));
     assertEquals(54321 * megabyte, calculator.getRegionSize(Bytes.toBytes("region2")));
     assertEquals(1232 * megabyte, calculator.getRegionSize(Bytes.toBytes("region3")));
+
     // if regionCalculator does not know about a region, it should return 0
-    assertEquals(0 * megabyte, calculator.getRegionSize(Bytes.toBytes("otherTableRegion")));
+    assertEquals(0, calculator.getRegionSize(Bytes.toBytes("otherTableRegion")));
 
     assertEquals(3, calculator.getRegionSizeMap().size());
   }
@@ -105,24 +108,37 @@ public void testDisabled() throws Exception {
     // then disabled calculator.
     configuration.setBoolean(RegionSizeCalculator.ENABLE_REGIONSIZECALCULATOR, false);
     RegionSizeCalculator disabledCalculator = new RegionSizeCalculator(table, admin);
-    assertEquals(0 * megabyte, disabledCalculator.getRegionSize(Bytes.toBytes(regionName)));
-
+    assertEquals(0, disabledCalculator.getRegionSize(Bytes.toBytes(regionName)));
     assertEquals(0, disabledCalculator.getRegionSizeMap().size());
   }
 
+  @Test
+  public void testRegionWithNullServerName() throws Exception {
+    RegionLocator regionLocator =
+      mockRegionLocator(null, Collections.singletonList("someBigRegion"));
+    Admin admin = mockAdmin(mockRegion("someBigRegion", Integer.MAX_VALUE));
+    RegionSizeCalculator calculator = new RegionSizeCalculator(regionLocator, admin);
+    assertEquals(0, calculator.getRegionSize(Bytes.toBytes("someBigRegion")));
+  }
+
   /**
    * Makes some table with given region names.
    */
   private RegionLocator mockRegionLocator(String... regionNames) throws IOException {
+    return mockRegionLocator(sn, Arrays.asList(regionNames));
+  }
+
+  private RegionLocator mockRegionLocator(ServerName serverName, List<String> regionNames)
+    throws IOException {
     RegionLocator mockedTable = Mockito.mock(RegionLocator.class);
     when(mockedTable.getName()).thenReturn(TableName.valueOf("sizeTestTable"));
-    List<HRegionLocation> regionLocations = new ArrayList<>(regionNames.length);
+    List<HRegionLocation> regionLocations = new ArrayList<>(regionNames.size());
     when(mockedTable.getAllRegionLocations()).thenReturn(regionLocations);
 
     for (String regionName : regionNames) {
       RegionInfo info = Mockito.mock(RegionInfo.class);
       when(info.getRegionName()).thenReturn(Bytes.toBytes(regionName));
-      regionLocations.add(new HRegionLocation(info, sn));
+      regionLocations.add(new HRegionLocation(info, serverName));
     }
 
     return mockedTable;
@@ -133,10 +149,7 @@ private RegionLocator mockRegionLocator(String... regionNames) throws IOExceptio
    */
   private Admin mockAdmin(RegionMetrics... regionLoadArray) throws Exception {
     Admin mockAdmin = Mockito.mock(Admin.class);
-    List<RegionMetrics> regionLoads = new ArrayList<>();
-    for (RegionMetrics regionLoad : regionLoadArray) {
-      regionLoads.add(regionLoad);
-    }
+    List<RegionMetrics> regionLoads = new ArrayList<>(Arrays.asList(regionLoadArray));
     when(mockAdmin.getConfiguration()).thenReturn(configuration);
     when(mockAdmin.getRegionMetrics(sn, TableName.valueOf("sizeTestTable")))
       .thenReturn(regionLoads);