From 8eea05235929a16706a83b759d98f9336f10fd49 Mon Sep 17 00:00:00 2001 From: niuyulin Date: Sun, 11 Oct 2020 22:02:37 -0500 Subject: [PATCH] HBASE-25093 the RSGroupBasedLoadBalancer#retainAssignment throws NPE (#2450) Signed-off-by: Duo Zhang --- .../favored/FavoredNodeLoadBalancer.java | 3 +++ .../hadoop/hbase/master/LoadBalancer.java | 5 ++-- .../master/assignment/AssignmentManager.java | 6 +---- .../master/balancer/BaseLoadBalancer.java | 27 +++++++++++-------- .../balancer/FavoredStochasticBalancer.java | 10 ++++--- .../rsgroup/RSGroupBasedLoadBalancer.java | 23 ++++++++-------- .../apache/hadoop/hbase/TestZooKeeper.java | 3 +++ 7 files changed, 45 insertions(+), 32 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java index 8cde76e07c6..60a2c6cae13 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java @@ -22,12 +22,14 @@ import static org.apache.hadoop.hbase.favored.FavoredNodesPlan.Position.PRIMARY; import static org.apache.hadoop.hbase.favored.FavoredNodesPlan.Position.SECONDARY; import static org.apache.hadoop.hbase.favored.FavoredNodesPlan.Position.TERTIARY; +import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; + import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.HBaseInterfaceAudience; @@ -161,6 +163,7 @@ public class FavoredNodeLoadBalancer extends BaseLoadBalancer implements Favored } @Override + @NonNull public Map> roundRobinAssignment(List regions, List servers) throws HBaseIOException { Map> assignmentMap; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java index b7ec1a3aa1b..90cb3946f8b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java @@ -18,7 +18,7 @@ */ package org.apache.hadoop.hbase.master; -import edu.umd.cs.findbugs.annotations.Nullable; +import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.util.List; import java.util.Map; @@ -110,6 +110,7 @@ public interface LoadBalancer extends Configurable, Stoppable, ConfigurationObse * Perform a Round Robin assignment of regions. * @return Map of servername to regioninfos */ + @NonNull Map> roundRobinAssignment(List regions, List servers) throws IOException; @@ -117,7 +118,7 @@ public interface LoadBalancer extends Configurable, Stoppable, ConfigurationObse * Assign regions to the previously hosting region server * @return List of plans */ - @Nullable + @NonNull Map> retainAssignment(Map regions, List servers) throws IOException; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java index fb64514a337..f23d17026f3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java @@ -2165,12 +2165,8 @@ public class AssignmentManager { final ProcedureEvent[] events = new ProcedureEvent[regions.size()]; final long st = System.currentTimeMillis(); - if (plan == null) { - throw new HBaseIOException("unable to compute plans for regions=" + regions.size()); - } - if (plan.isEmpty()) { - return; + throw new HBaseIOException("unable to compute plans for regions=" + regions.size()); } int evcount = 0; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java index 6a27a6a0568..a47bff26a09 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java @@ -18,6 +18,7 @@ */ package org.apache.hadoop.hbase.master.balancer; +import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -1132,11 +1133,9 @@ public abstract class BaseLoadBalancer implements LoadBalancer { * If master is configured to carry system tables only, in here is * where we figure what to assign it. */ + @NonNull protected Map> assignMasterSystemRegions( Collection regions, List servers) { - if (servers == null || regions == null || regions.isEmpty()) { - return null; - } Map> assignments = new TreeMap<>(); if (this.maintenanceMode || this.onlySystemTablesOnMaster) { if (masterServerName != null && servers.contains(masterServerName)) { @@ -1267,15 +1266,16 @@ public abstract class BaseLoadBalancer implements LoadBalancer { * * @param regions all regions * @param servers all servers - * @return map of server to the regions it should take, or null if no - * assignment is possible (ie. no regions or no servers) + * @return map of server to the regions it should take, or emptyMap if no + * assignment is possible (ie. no servers) */ @Override + @NonNull public Map> roundRobinAssignment(List regions, List servers) throws HBaseIOException { metricsBalancer.incrMiscInvocations(); Map> assignments = assignMasterSystemRegions(regions, servers); - if (assignments != null && !assignments.isEmpty()) { + if (!assignments.isEmpty()) { servers = new ArrayList<>(servers); // Guarantee not to put other regions on master servers.remove(masterServerName); @@ -1285,14 +1285,17 @@ public abstract class BaseLoadBalancer implements LoadBalancer { regions.removeAll(masterRegions); } } - if (this.maintenanceMode || regions == null || regions.isEmpty()) { + /** + * only need assign system table + */ + if (this.maintenanceMode || regions.isEmpty()) { return assignments; } int numServers = servers == null ? 0 : servers.size(); if (numServers == 0) { LOG.warn("Wanted to do round robin assignment but no servers to assign to"); - return null; + return Collections.emptyMap(); } // TODO: instead of retainAssignment() and roundRobinAssignment(), we should just run the @@ -1407,15 +1410,17 @@ public abstract class BaseLoadBalancer implements LoadBalancer { * * @param regions regions and existing assignment from meta * @param servers available servers - * @return map of servers and regions to be assigned to them + * @return map of servers and regions to be assigned to them, or emptyMap if no + * assignment is possible (ie. no servers) */ @Override + @NonNull public Map> retainAssignment(Map regions, List servers) throws HBaseIOException { // Update metrics metricsBalancer.incrMiscInvocations(); Map> assignments = assignMasterSystemRegions(regions.keySet(), servers); - if (assignments != null && !assignments.isEmpty()) { + if (!assignments.isEmpty()) { servers = new ArrayList<>(servers); // Guarantee not to put other regions on master servers.remove(masterServerName); @@ -1430,7 +1435,7 @@ public abstract class BaseLoadBalancer implements LoadBalancer { int numServers = servers == null ? 0 : servers.size(); if (numServers == 0) { LOG.warn("Wanted to do retain assignment but no servers to assign to"); - return null; + return Collections.emptyMap(); } if (numServers == 1) { // Only one server, nothing fancy we can do here ServerName server = servers.get(0); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java index 57fd73d298c..4108a5a201d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java @@ -23,6 +23,7 @@ import static org.apache.hadoop.hbase.favored.FavoredNodesPlan.Position.PRIMARY; import static org.apache.hadoop.hbase.favored.FavoredNodesPlan.Position.SECONDARY; import static org.apache.hadoop.hbase.favored.FavoredNodesPlan.Position.TERTIARY; +import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; @@ -31,6 +32,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; + import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.ServerMetrics; import org.apache.hadoop.hbase.ServerName; @@ -109,6 +111,7 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements * secondary and tertiary as per favored nodes constraints. */ @Override + @NonNull public Map> roundRobinAssignment(List regions, List servers) throws HBaseIOException { @@ -116,7 +119,7 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements Set regionSet = Sets.newHashSet(regions); Map> assignmentMap = assignMasterSystemRegions(regions, servers); - if (assignmentMap != null && !assignmentMap.isEmpty()) { + if (!assignmentMap.isEmpty()) { servers = new ArrayList<>(servers); // Guarantee not to put other regions on master servers.remove(masterServerName); @@ -367,14 +370,15 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements * Reuse BaseLoadBalancer's retainAssignment, but generate favored nodes when its missing. */ @Override + @NonNull public Map> retainAssignment(Map regions, List servers) throws HBaseIOException { Map> assignmentMap = Maps.newHashMap(); Map> result = super.retainAssignment(regions, servers); - if (result == null || result.isEmpty()) { + if (result.isEmpty()) { LOG.warn("Nothing to assign to, probably no servers or no regions"); - return null; + return result; } // Guarantee not to put other regions on master diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java index 50ddb416e91..db61c01dec9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.rsgroup; +import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; @@ -174,25 +175,25 @@ public class RSGroupBasedLoadBalancer implements LoadBalancer { } @Override - public Map> roundRobinAssignment( - List regions, List servers) throws IOException { + @NonNull + public Map> roundRobinAssignment(List regions, + List servers) throws IOException { Map> assignments = Maps.newHashMap(); List, List>> pairs = generateGroupAssignments(regions, servers); for (Pair, List> pair : pairs) { - Map> result = this.internalBalancer - .roundRobinAssignment(pair.getFirst(), pair.getSecond()); - if (result != null) { - result.forEach((server, regionInfos) -> - assignments.computeIfAbsent(server, s -> Lists.newArrayList()).addAll(regionInfos)); - } + Map> result = + this.internalBalancer.roundRobinAssignment(pair.getFirst(), pair.getSecond()); + result.forEach((server, regionInfos) -> assignments + .computeIfAbsent(server, s -> Lists.newArrayList()).addAll(regionInfos)); } return assignments; } @Override + @NonNull public Map> retainAssignment(Map regions, - List servers) throws HBaseIOException { + List servers) throws HBaseIOException { try { Map> assignments = new TreeMap<>(); List, List>> pairs = @@ -203,8 +204,8 @@ public class RSGroupBasedLoadBalancer implements LoadBalancer { regionList.forEach(r -> currentAssignmentMap.put(r, regions.get(r))); Map> pairResult = this.internalBalancer.retainAssignment(currentAssignmentMap, pair.getSecond()); - pairResult.forEach((server, rs) -> - assignments.computeIfAbsent(server, s -> Lists.newArrayList()).addAll(rs)); + pairResult.forEach((server, rs) -> assignments + .computeIfAbsent(server, s -> Lists.newArrayList()).addAll(rs)); } return assignments; } catch (IOException e) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java index 4bbb3ed9c17..c0eacae0a18 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java @@ -21,8 +21,10 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import edu.umd.cs.findbugs.annotations.NonNull; import java.util.List; import java.util.Map; + import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.client.Admin; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; @@ -280,6 +282,7 @@ public class TestZooKeeper { static boolean retainAssignCalled = false; @Override + @NonNull public Map> retainAssignment( Map regions, List servers) throws HBaseIOException { retainAssignCalled = true;