Skip to content

Commit af338d2

Browse files
comnetworkvirajjasani
comnetwork
authored and
virajjasani
committed
HBASE-26712 Balancer encounters NPE in rare case (#4112) (#4092)
Signed-off-by: Viraj Jasani <vjasani@apache.org>
1 parent 42bb0af commit af338d2

File tree

3 files changed

+251
-3
lines changed

3 files changed

+251
-3
lines changed

‎hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,7 @@ public class HMaster extends HRegionServer implements MasterServices {
343343

344344
private LoadBalancer balancer;
345345
private BalancerChore balancerChore;
346+
private static boolean disableBalancerChoreForTest = false;
346347
private RegionNormalizerManager regionNormalizerManager;
347348
private ClusterStatusChore clusterStatusChore;
348349
private ClusterStatusPublisher clusterStatusPublisherChore = null;
@@ -1088,7 +1089,9 @@ private void finishActiveMasterInitialization(MonitoredTask status)
10881089
this.clusterStatusChore = new ClusterStatusChore(this, balancer);
10891090
getChoreService().scheduleChore(clusterStatusChore);
10901091
this.balancerChore = new BalancerChore(this);
1091-
getChoreService().scheduleChore(balancerChore);
1092+
if (!disableBalancerChoreForTest) {
1093+
getChoreService().scheduleChore(balancerChore);
1094+
}
10921095
if (regionNormalizerManager != null) {
10931096
getChoreService().scheduleChore(regionNormalizerManager.getRegionNormalizerChore());
10941097
}
@@ -4015,4 +4018,23 @@ MasterRegion getMasterRegion() {
40154018
public Collection<ServerName> getLiveRegionServers() {
40164019
return regionServerTracker.getRegionServers();
40174020
}
4021+
4022+
@RestrictedApi(explanation = "Should only be called in tests", link = "",
4023+
allowedOnPath = ".*/src/test/.*")
4024+
void setLoadBalancer(LoadBalancer loadBalancer) {
4025+
this.balancer = loadBalancer;
4026+
}
4027+
4028+
@RestrictedApi(explanation = "Should only be called in tests", link = "",
4029+
allowedOnPath = ".*/src/test/.*")
4030+
void setAssignmentManager(AssignmentManager assignmentManager) {
4031+
this.assignmentManager = assignmentManager;
4032+
}
4033+
4034+
@RestrictedApi(explanation = "Should only be called in tests", link = "",
4035+
allowedOnPath = ".*/src/test/.*")
4036+
static void setDisableBalancerChoreForTest(boolean disable) {
4037+
disableBalancerChoreForTest = disable;
4038+
}
4039+
40184040
}

‎hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -823,9 +823,9 @@ public Future<byte[]> moveAsync(RegionPlan regionPlan) throws HBaseIOException {
823823
public Future<byte[]> balance(RegionPlan regionPlan) throws HBaseIOException {
824824
ServerName current =
825825
this.getRegionStates().getRegionAssignments().get(regionPlan.getRegionInfo());
826-
if (!current.equals(regionPlan.getSource())) {
826+
if (current == null || !current.equals(regionPlan.getSource())) {
827827
LOG.debug("Skip region plan {}, source server not match, current region location is {}",
828-
regionPlan, current);
828+
regionPlan, current == null ? "(null)" : current);
829829
return null;
830830
}
831831
return moveAsync(regionPlan);

‎hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterBalancerNPE.java

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.apache.hadoop.hbase.master;
19+
20+
import static org.junit.Assert.assertTrue;
21+
22+
import java.io.IOException;
23+
import java.util.ArrayList;
24+
import java.util.Arrays;
25+
import java.util.List;
26+
import java.util.Map;
27+
import java.util.concurrent.CyclicBarrier;
28+
import java.util.concurrent.atomic.AtomicReference;
29+
30+
import org.apache.hadoop.hbase.HBaseClassTestRule;
31+
import org.apache.hadoop.hbase.HBaseTestingUtility;
32+
import org.apache.hadoop.hbase.HConstants;
33+
import org.apache.hadoop.hbase.ServerName;
34+
import org.apache.hadoop.hbase.TableName;
35+
import org.apache.hadoop.hbase.client.RegionInfo;
36+
import org.apache.hadoop.hbase.master.assignment.AssignmentManager;
37+
import org.apache.hadoop.hbase.master.balancer.LoadBalancerFactory;
38+
import org.apache.hadoop.hbase.master.balancer.StochasticLoadBalancer;
39+
import org.apache.hadoop.hbase.testclassification.MasterTests;
40+
import org.apache.hadoop.hbase.testclassification.MediumTests;
41+
import org.apache.hadoop.hbase.util.Bytes;
42+
import org.junit.After;
43+
import org.junit.Before;
44+
import org.junit.ClassRule;
45+
import org.junit.Rule;
46+
import org.junit.Test;
47+
import org.junit.experimental.categories.Category;
48+
import org.junit.rules.TestName;
49+
import org.mockito.Mockito;
50+
import org.mockito.invocation.InvocationOnMock;
51+
52+
@Category({ MasterTests.class, MediumTests.class })
53+
public class TestMasterBalancerNPE {
54+
55+
@ClassRule
56+
public static final HBaseClassTestRule CLASS_RULE =
57+
HBaseClassTestRule.forClass(TestMasterBalancerNPE.class);
58+
59+
private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
60+
private static final byte[] FAMILYNAME = Bytes.toBytes("fam");
61+
@Rule
62+
public TestName name = new TestName();
63+
64+
@Before
65+
public void setupConfiguration() {
66+
/**
67+
* Make {@link BalancerChore} not run,so does not disrupt the test.
68+
*/
69+
HMaster.setDisableBalancerChoreForTest(true);
70+
TEST_UTIL.getConfiguration().set(HConstants.HBASE_MASTER_LOADBALANCER_CLASS,
71+
MyLoadBalancer.class.getName());
72+
}
73+
74+
@After
75+
public void shutdown() throws Exception {
76+
HMaster.setDisableBalancerChoreForTest(false);
77+
TEST_UTIL.getConfiguration().set(HConstants.HBASE_MASTER_LOADBALANCER_CLASS,
78+
LoadBalancerFactory.getDefaultLoadBalancerClass().getName());
79+
TEST_UTIL.shutdownMiniCluster();
80+
}
81+
82+
/**
83+
* This test is for HBASE-26712, to make the region is unassigned just before
84+
* {@link AssignmentManager#balance} is invoked on the region.
85+
*/
86+
@Test
87+
public void testBalancerNPE() throws Exception {
88+
TEST_UTIL.startMiniCluster(2);
89+
TEST_UTIL.getAdmin().balancerSwitch(false, true);
90+
TableName tableName = createTable(name.getMethodName());
91+
final HMaster master = TEST_UTIL.getHBaseCluster().getMaster();
92+
List<RegionInfo> regionInfos = TEST_UTIL.getAdmin().getRegions(tableName);
93+
assertTrue(regionInfos.size() == 1);
94+
final ServerName serverName1 =
95+
TEST_UTIL.getMiniHBaseCluster().getRegionServer(0).getServerName();
96+
final ServerName serverName2 =
97+
TEST_UTIL.getMiniHBaseCluster().getRegionServer(1).getServerName();
98+
99+
final RegionInfo regionInfo = regionInfos.get(0);
100+
101+
MyLoadBalancer loadBalancer = (MyLoadBalancer)master.getLoadBalancer();
102+
MyLoadBalancer spiedLoadBalancer = Mockito.spy(loadBalancer);
103+
final AtomicReference<RegionPlan> regionPlanRef = new AtomicReference<RegionPlan>();
104+
105+
/**
106+
* Mock {@link StochasticLoadBalancer#balanceTable} to return the {@link RegionPlan} to move the
107+
* only region to the other RegionServer.
108+
*/
109+
Mockito.doAnswer((InvocationOnMock invocation) -> {
110+
@SuppressWarnings("unchecked")
111+
Map<ServerName, List<RegionInfo>> regionServerNameToRegionInfos =
112+
(Map<ServerName, List<RegionInfo>>) invocation.getArgument(1);
113+
114+
115+
List<ServerName> assignedRegionServerNames = new ArrayList<ServerName>();
116+
for (Map.Entry<ServerName, List<RegionInfo>> entry : regionServerNameToRegionInfos
117+
.entrySet()) {
118+
if (entry.getValue()!= null) {
119+
entry.getValue().stream().forEach((reginInfo) -> {
120+
if(reginInfo.getTable().equals(tableName)) {assignedRegionServerNames.add(entry.getKey());}});
121+
}
122+
}
123+
assertTrue(assignedRegionServerNames.size() == 1);
124+
ServerName assignedRegionServerName = assignedRegionServerNames.get(0);
125+
ServerName notAssignedRegionServerName =
126+
assignedRegionServerName.equals(serverName1) ? serverName2 : serverName1;
127+
RegionPlan regionPlan =
128+
new RegionPlan(regionInfo, assignedRegionServerName, notAssignedRegionServerName);
129+
regionPlanRef.set(regionPlan);
130+
return Arrays.asList(regionPlan);
131+
}).when(spiedLoadBalancer).balanceTable(Mockito.eq(HConstants.ENSEMBLE_TABLE_NAME),
132+
Mockito.anyMap());
133+
134+
AssignmentManager assignmentManager = master.getAssignmentManager();
135+
final AssignmentManager spiedAssignmentManager = Mockito.spy(assignmentManager);
136+
final CyclicBarrier cyclicBarrier = new CyclicBarrier(2);
137+
138+
/**
139+
* Override {@link AssignmentManager#balance} to invoke real {@link AssignmentManager#balance}
140+
* after the region is successfully unassigned.
141+
*/
142+
Mockito.doAnswer((InvocationOnMock invocation) -> {
143+
RegionPlan regionPlan = invocation.getArgument(0);
144+
RegionPlan referedRegionPlan = regionPlanRef.get();
145+
assertTrue(referedRegionPlan != null);
146+
if (referedRegionPlan.equals(regionPlan)) {
147+
/**
148+
* To make {@link AssignmentManager#unassign} could be invoked just before
149+
* {@link AssignmentManager#balance} is invoked.
150+
*/
151+
cyclicBarrier.await();
152+
/**
153+
* After {@link AssignmentManager#unassign} is completed,we could invoke
154+
* {@link AssignmentManager#balance}.
155+
*/
156+
cyclicBarrier.await();
157+
}
158+
/**
159+
* Before HBASE-26712,here may throw NPE.
160+
*/
161+
return invocation.callRealMethod();
162+
}).when(spiedAssignmentManager).balance(Mockito.any());
163+
164+
165+
try {
166+
final AtomicReference<Throwable> exceptionRef = new AtomicReference<Throwable>(null);
167+
Thread unassignThread = new Thread(() -> {
168+
try {
169+
/**
170+
* To invoke {@link AssignmentManager#unassign} just before
171+
* {@link AssignmentManager#balance} is invoked.
172+
*/
173+
cyclicBarrier.await();
174+
spiedAssignmentManager.unassign(regionInfo);
175+
assertTrue(spiedAssignmentManager.getRegionStates().getRegionAssignments()
176+
.get(regionInfo) == null);
177+
/**
178+
* After {@link AssignmentManager#unassign} is completed,we could invoke
179+
* {@link AssignmentManager#balance}.
180+
*/
181+
cyclicBarrier.await();
182+
} catch (Exception e) {
183+
exceptionRef.set(e);
184+
}
185+
});
186+
unassignThread.setName("UnassignThread");
187+
unassignThread.start();
188+
189+
master.setLoadBalancer(spiedLoadBalancer);
190+
master.setAssignmentManager(spiedAssignmentManager);
191+
/**
192+
* enable balance
193+
*/
194+
TEST_UTIL.getAdmin().balancerSwitch(true, false);
195+
/**
196+
* Before HBASE-26712,here invokes {@link AssignmentManager#balance(RegionPlan)}
197+
* which may throw NPE.
198+
*/
199+
master.balanceOrUpdateMetrics();
200+
201+
unassignThread.join();
202+
assertTrue(exceptionRef.get() == null);
203+
} finally {
204+
master.setLoadBalancer(loadBalancer);
205+
master.setAssignmentManager(assignmentManager);
206+
}
207+
}
208+
209+
private TableName createTable(String table) throws IOException {
210+
TableName tableName = TableName.valueOf(table);
211+
TEST_UTIL.createTable(tableName, FAMILYNAME);
212+
return tableName;
213+
}
214+
215+
/**
216+
* Define this class because the test needs to override
217+
* {@link StochasticLoadBalancer#balanceTable}, which is protected.
218+
*/
219+
static class MyLoadBalancer extends StochasticLoadBalancer{
220+
@Override
221+
protected List<RegionPlan> balanceTable(TableName tableName,
222+
Map<ServerName, List<RegionInfo>> loadOfOneTable) {
223+
return super.balanceTable(tableName, loadOfOneTable);
224+
}
225+
}
226+
}

0 commit comments

Comments
 (0)