You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by zh...@apache.org on 2020/10/19 02:43:36 UTC
[hbase] branch branch-2.2 updated: HBASE-25093 the
RSGroupBasedLoadBalancer#retainAssignment throws NPE (#2553)
This is an automated email from the ASF dual-hosted git repository.
zhangduo pushed a commit to branch branch-2.2
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2.2 by this push:
new 06d5aad HBASE-25093 the RSGroupBasedLoadBalancer#retainAssignment throws NPE (#2553)
06d5aad is described below
commit 06d5aadd9292c6cf11445372b9a38ca273318e3e
Author: niuyulin <ny...@163.com>
AuthorDate: Mon Oct 19 10:36:08 2020 +0800
HBASE-25093 the RSGroupBasedLoadBalancer#retainAssignment throws NPE (#2553)
Signed-off-by: Duo Zhang <zh...@apache.org>
---
.../hbase/rsgroup/RSGroupBasedLoadBalancer.java | 25 ++++++++++----------
.../hbase/favored/FavoredNodeLoadBalancer.java | 2 ++
.../apache/hadoop/hbase/master/LoadBalancer.java | 17 ++++++--------
.../hbase/master/assignment/AssignmentManager.java | 4 +---
.../hbase/master/balancer/BaseLoadBalancer.java | 27 +++++++++++++---------
.../master/balancer/FavoredStochasticBalancer.java | 9 +++++---
.../org/apache/hadoop/hbase/TestZooKeeper.java | 2 ++
7 files changed, 47 insertions(+), 39 deletions(-)
diff --git a/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java b/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java
index cc163ac..207102f 100644
--- a/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java
+++ b/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java
@@ -18,6 +18,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;
@@ -160,8 +161,9 @@ public class RSGroupBasedLoadBalancer implements RSGroupableBalancer {
}
@Override
+ @NonNull
public Map<ServerName, List<RegionInfo>> roundRobinAssignment(List<RegionInfo> regions,
- List<ServerName> servers) throws HBaseIOException {
+ List<ServerName> servers) throws HBaseIOException {
Map<ServerName, List<RegionInfo>> assignments = Maps.newHashMap();
ListMultimap<String, RegionInfo> regionMap = ArrayListMultimap.create();
ListMultimap<String, ServerName> serverMap = ArrayListMultimap.create();
@@ -169,15 +171,13 @@ public class RSGroupBasedLoadBalancer implements RSGroupableBalancer {
for (String groupKey : regionMap.keySet()) {
if (regionMap.get(groupKey).size() > 0) {
Map<ServerName, List<RegionInfo>> result = this.internalBalancer
- .roundRobinAssignment(regionMap.get(groupKey), serverMap.get(groupKey));
- if (result != null) {
- if (result.containsKey(LoadBalancer.BOGUS_SERVER_NAME) &&
- assignments.containsKey(LoadBalancer.BOGUS_SERVER_NAME)) {
- assignments.get(LoadBalancer.BOGUS_SERVER_NAME)
+ .roundRobinAssignment(regionMap.get(groupKey), serverMap.get(groupKey));
+ if (result.containsKey(LoadBalancer.BOGUS_SERVER_NAME)
+ && assignments.containsKey(LoadBalancer.BOGUS_SERVER_NAME)) {
+ assignments.get(LoadBalancer.BOGUS_SERVER_NAME)
.addAll(result.get(LoadBalancer.BOGUS_SERVER_NAME));
- } else {
- assignments.putAll(result);
- }
+ } else {
+ assignments.putAll(result);
}
}
}
@@ -185,8 +185,9 @@ public class RSGroupBasedLoadBalancer implements RSGroupableBalancer {
}
@Override
+ @NonNull
public Map<ServerName, List<RegionInfo>> retainAssignment(Map<RegionInfo, ServerName> regions,
- List<ServerName> servers) throws HBaseIOException {
+ List<ServerName> servers) throws HBaseIOException {
try {
Map<ServerName, List<RegionInfo>> assignments = new TreeMap<>();
ListMultimap<String, RegionInfo> groupToRegion = ArrayListMultimap.create();
@@ -208,14 +209,14 @@ public class RSGroupBasedLoadBalancer implements RSGroupableBalancer {
}
if (candidateList.size() > 0) {
assignments
- .putAll(this.internalBalancer.retainAssignment(currentAssignmentMap, candidateList));
+ .putAll(this.internalBalancer.retainAssignment(currentAssignmentMap, candidateList));
} else {
if (LOG.isDebugEnabled()) {
LOG.debug("No available servers to assign regions: {}",
RegionInfo.getShortNameToLog(regionList));
}
assignments.computeIfAbsent(LoadBalancer.BOGUS_SERVER_NAME, s -> new ArrayList<>())
- .addAll(regionList);
+ .addAll(regionList);
}
}
return assignments;
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 5a4ccd2..5845f2c 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,6 +22,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.HashMap;
@@ -161,6 +162,7 @@ public class FavoredNodeLoadBalancer extends BaseLoadBalancer implements Favored
}
@Override
+ @NonNull
public Map<ServerName, List<RegionInfo>> roundRobinAssignment(List<RegionInfo> regions,
List<ServerName> servers) throws HBaseIOException {
Map<ServerName, List<RegionInfo>> 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 84b8adc..daa4083 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;
@@ -103,10 +103,9 @@ public interface LoadBalancer extends Configurable, Stoppable, ConfigurationObse
* @param servers
* @return Map of servername to regioninfos
*/
- Map<ServerName, List<RegionInfo>> roundRobinAssignment(
- List<RegionInfo> regions,
- List<ServerName> servers
- ) throws HBaseIOException;
+ @NonNull
+ Map<ServerName, List<RegionInfo>> roundRobinAssignment(List<RegionInfo> regions,
+ List<ServerName> servers) throws HBaseIOException;
/**
* Assign regions to the previously hosting region server
@@ -114,11 +113,9 @@ public interface LoadBalancer extends Configurable, Stoppable, ConfigurationObse
* @param servers
* @return List of plans
*/
- @Nullable
- Map<ServerName, List<RegionInfo>> retainAssignment(
- Map<RegionInfo, ServerName> regions,
- List<ServerName> servers
- ) throws HBaseIOException;
+ @NonNull
+ Map<ServerName, List<RegionInfo>> retainAssignment(Map<RegionInfo, ServerName> regions,
+ List<ServerName> servers) throws HBaseIOException;
/**
* Get a random region server from the list
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 8afe691..0c6e03e 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
@@ -2055,12 +2055,10 @@ public class AssignmentManager {
final ProcedureEvent<?>[] events = new ProcedureEvent[regions.size()];
final long st = System.currentTimeMillis();
- if (plan == null) {
+ if (plan.isEmpty()) {
throw new HBaseIOException("unable to compute plans for regions=" + regions.size());
}
- if (plan.isEmpty()) return;
-
int evcount = 0;
for (Map.Entry<ServerName, List<RegionInfo>> entry: plan.entrySet()) {
final ServerName server = entry.getKey();
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 d03e07a..246f903 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;
@@ -1120,11 +1121,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<ServerName, List<RegionInfo>> assignMasterSystemRegions(
Collection<RegionInfo> regions, List<ServerName> servers) {
- if (servers == null || regions == null || regions.isEmpty()) {
- return null;
- }
Map<ServerName, List<RegionInfo>> assignments = new TreeMap<>();
if (this.maintenanceMode || this.onlySystemTablesOnMaster) {
if (masterServerName != null && servers.contains(masterServerName)) {
@@ -1237,15 +1236,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<ServerName, List<RegionInfo>> roundRobinAssignment(List<RegionInfo> regions,
List<ServerName> servers) throws HBaseIOException {
metricsBalancer.incrMiscInvocations();
Map<ServerName, List<RegionInfo>> 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);
@@ -1255,14 +1255,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
@@ -1377,15 +1380,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<ServerName, List<RegionInfo>> retainAssignment(Map<RegionInfo, ServerName> regions,
List<ServerName> servers) throws HBaseIOException {
// Update metrics
metricsBalancer.incrMiscInvocations();
Map<ServerName, List<RegionInfo>> 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);
@@ -1400,7 +1405,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 add0f1c..2a2857e 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;
@@ -109,6 +110,7 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements
* secondary and tertiary as per favored nodes constraints.
*/
@Override
+ @NonNull
public Map<ServerName, List<RegionInfo>> roundRobinAssignment(List<RegionInfo> regions,
List<ServerName> servers) throws HBaseIOException {
@@ -116,7 +118,7 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements
Set<RegionInfo> regionSet = Sets.newHashSet(regions);
Map<ServerName, List<RegionInfo>> 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 +369,15 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements
* Reuse BaseLoadBalancer's retainAssignment, but generate favored nodes when its missing.
*/
@Override
+ @NonNull
public Map<ServerName, List<RegionInfo>> retainAssignment(Map<RegionInfo, ServerName> regions,
List<ServerName> servers) throws HBaseIOException {
Map<ServerName, List<RegionInfo>> assignmentMap = Maps.newHashMap();
Map<ServerName, List<RegionInfo>> 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/test/java/org/apache/hadoop/hbase/TestZooKeeper.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java
index a419e22..2f4c230 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,6 +21,7 @@ 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;
@@ -276,6 +277,7 @@ public class TestZooKeeper {
static boolean retainAssignCalled = false;
@Override
+ @NonNull
public Map<ServerName, List<RegionInfo>> retainAssignment(
Map<RegionInfo, ServerName> regions, List<ServerName> servers) throws HBaseIOException {
retainAssignCalled = true;