This is an automated email from the ASF dual-hosted git repository.
ndimiduk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/master by this push:
new 1c41e861f8f HBASE-28354 RegionSizeCalculator throws NPE when regions
are in transition (#5699)
1c41e861f8f is described below
commit 1c41e861f8f54206ad98ee9b8ab943d55210c65f
Author: Ahmad Alhour <[email protected]>
AuthorDate: Thu Feb 29 14:55:25 2024 +0100
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 <[email protected]>
Signed-off-by: Hui Ruan <[email protected]>
---
.../hbase/mapreduce/RegionSizeCalculator.java | 15 +++++------
.../hbase/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 4d027196a8f..cc36ef5deb4 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.io.IOException;
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.apache.yetus.audience.InterfaceAudience;
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 @@ public class RegionSizeCalculator {
}
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) {
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 f841bdbb61d..2fda536438a 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 static org.mockito.Mockito.when;
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 class TestRegionSizeCalculator {
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 class TestRegionSizeCalculator {
// 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 @@ public class TestRegionSizeCalculator {
*/
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);
