From 441b2fe8ba505aff18333a468a85d0d2b4e38c50 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Thu, 29 Feb 2024 14:55:25 +0100 Subject: [PATCH] HBASE-28354 RegionSizeCalculator throws NPE when regions are in transition (#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 Signed-off-by: Hui Ruan --- .../hbase/mapreduce/RegionSizeCalculator.java | 15 +++++---- .../mapreduce/TestRegionSizeCalculator.java | 31 +++++++++++++------ 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/RegionSizeCalculator.java b/hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/RegionSizeCalculator.java index 4d027196a8fe..cc36ef5deb48 100644 --- a/hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/RegionSizeCalculator.java +++ b/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 getRegionServersOfTable(RegionLocator regionLocator) throws IOException { - - Set 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) { diff --git a/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestRegionSizeCalculator.java b/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestRegionSizeCalculator.java index f841bdbb61dc..2fda536438a7 100644 --- a/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestRegionSizeCalculator.java +++ b/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 regionNames) + throws IOException { RegionLocator mockedTable = Mockito.mock(RegionLocator.class); when(mockedTable.getName()).thenReturn(TableName.valueOf("sizeTestTable")); - List regionLocations = new ArrayList<>(regionNames.length); + List 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 regionLoads = new ArrayList<>(); - for (RegionMetrics regionLoad : regionLoadArray) { - regionLoads.add(regionLoad); - } + List regionLoads = new ArrayList<>(Arrays.asList(regionLoadArray)); when(mockAdmin.getConfiguration()).thenReturn(configuration); when(mockAdmin.getRegionMetrics(sn, TableName.valueOf("sizeTestTable"))) .thenReturn(regionLoads);