Skip to content

Commit ce3a37e

Browse files
bbeaudreaultbbeaudreault
committed
HBASE-26790 getAllRegionLocations can cache locations with null hostname (#4575)
Signed-off-by: Andrew Purtell <[email protected]>
1 parent 207b7d0 commit ce3a37e

4 files changed

Lines changed: 136 additions & 3 deletions

File tree

‎hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,12 @@ public CompletableFuture<List<HRegionLocation>> getAllRegionLocations() {
6262
}
6363
CompletableFuture<List<HRegionLocation>> future = AsyncMetaTableAccessor
6464
.getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName);
65-
addListener(future, (locs, error) -> locs
66-
.forEach(loc -> conn.getLocator().getNonMetaRegionLocator().addLocationToCache(loc)));
65+
addListener(future, (locs, error) -> locs.forEach(loc -> {
66+
// the cache assumes that all locations have a serverName. only add if that's true
67+
if (loc.getServerName() != null) {
68+
conn.getLocator().getNonMetaRegionLocator().addLocationToCache(loc);
69+
}
70+
}));
6771
return future;
6872
}
6973

‎hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,11 @@ public List<HRegionLocation> getAllRegionLocations() throws IOException {
7979
for (HRegionLocation location : locations.getRegionLocations()) {
8080
regions.add(location);
8181
}
82-
connection.cacheLocation(tableName, locations);
82+
RegionLocations cleaned = locations.removeElementsWithNullLocation();
83+
// above can return null if all locations had null location
84+
if (cleaned != null) {
85+
connection.cacheLocation(tableName, cleaned);
86+
}
8387
}
8488
return regions;
8589
}

‎hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncNonMetaRegionLocator.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import static org.junit.Assert.assertArrayEquals;
2727
import static org.junit.Assert.assertEquals;
2828
import static org.junit.Assert.assertNotNull;
29+
import static org.junit.Assert.assertNull;
2930
import static org.junit.Assert.assertSame;
3031

3132
import java.io.IOException;
@@ -41,6 +42,7 @@
4142
import org.apache.hadoop.hbase.HBaseClassTestRule;
4243
import org.apache.hadoop.hbase.HBaseTestingUtility;
4344
import org.apache.hadoop.hbase.HRegionLocation;
45+
import org.apache.hadoop.hbase.MetaTableAccessor;
4446
import org.apache.hadoop.hbase.NotServingRegionException;
4547
import org.apache.hadoop.hbase.RegionLocations;
4648
import org.apache.hadoop.hbase.ServerName;
@@ -52,6 +54,7 @@
5254
import org.apache.hadoop.hbase.testclassification.ClientTests;
5355
import org.apache.hadoop.hbase.testclassification.MediumTests;
5456
import org.apache.hadoop.hbase.util.Bytes;
57+
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
5558
import org.apache.hadoop.hbase.util.ServerRegionReplicaUtil;
5659
import org.junit.After;
5760
import org.junit.AfterClass;
@@ -64,6 +67,7 @@
6467
import org.slf4j.Logger;
6568
import org.slf4j.LoggerFactory;
6669

70+
import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
6771
import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
6872

6973
@Category({ MediumTests.class, ClientTests.class })
@@ -481,4 +485,66 @@ public void testCacheLocationWhenGetAllLocations() throws Exception {
481485
region.getStartKey()));
482486
}
483487
}
488+
489+
@Test
490+
public void testDoNotCacheLocationWithNullServerNameWhenGetAllLocations() throws Exception {
491+
createMultiRegionTable();
492+
AsyncConnectionImpl conn = (AsyncConnectionImpl) ConnectionFactory
493+
.createAsyncConnection(TEST_UTIL.getConfiguration()).get();
494+
List<RegionInfo> regions = TEST_UTIL.getAdmin().getRegions(TABLE_NAME);
495+
RegionInfo chosen = regions.get(0);
496+
497+
// re-populate region cache
498+
AsyncTableRegionLocator regionLocator = conn.getRegionLocator(TABLE_NAME);
499+
regionLocator.clearRegionLocationCache();
500+
regionLocator.getAllRegionLocations().get();
501+
502+
// expect all to be non-null at first
503+
int tries = 3;
504+
checkRegionsWithRetries(conn, regions, null, tries);
505+
506+
// clear servername from region info
507+
Put put = MetaTableAccessor.makePutFromRegionInfo(chosen, EnvironmentEdgeManager.currentTime());
508+
MetaTableAccessor.addEmptyLocation(put, 0);
509+
MetaTableAccessor.putsToMetaTable(TEST_UTIL.getConnection(), Lists.newArrayList(put));
510+
511+
// re-populate region cache again. check that we succeeded in nulling the servername
512+
regionLocator.clearRegionLocationCache();
513+
for (HRegionLocation loc : regionLocator.getAllRegionLocations().get()) {
514+
if (loc.getRegion().equals(chosen)) {
515+
assertNull(loc.getServerName());
516+
}
517+
}
518+
519+
// expect all but chosen to be non-null. chosen should be null because serverName was null
520+
checkRegionsWithRetries(conn, regions, chosen, tries);
521+
}
522+
523+
// caching of getAllRegionLocations is async. so we give it a couple tries
524+
private void checkRegionsWithRetries(AsyncConnectionImpl conn, List<RegionInfo> regions,
525+
RegionInfo chosen, int retries) throws InterruptedException {
526+
while (true) {
527+
try {
528+
checkRegions(conn, regions, chosen);
529+
break;
530+
} catch (AssertionError e) {
531+
if (retries-- <= 0) {
532+
throw e;
533+
}
534+
Thread.sleep(500);
535+
}
536+
}
537+
}
538+
539+
private void checkRegions(AsyncConnectionImpl conn, List<RegionInfo> regions, RegionInfo chosen) {
540+
for (RegionInfo region : regions) {
541+
RegionLocations fromCache = conn.getLocator().getNonMetaRegionLocator()
542+
.getRegionLocationInCache(TABLE_NAME, region.getStartKey());
543+
if (region.equals(chosen)) {
544+
assertNull(fromCache);
545+
} else {
546+
assertNotNull(fromCache);
547+
}
548+
}
549+
}
484550
}

‎hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRegionLocationCaching.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
package org.apache.hadoop.hbase.client;
1919

2020
import static org.junit.Assert.assertNotEquals;
21+
import static org.junit.Assert.assertNotNull;
2122
import static org.junit.Assert.assertNull;
2223
import static org.junit.Assert.assertTrue;
2324

@@ -26,15 +27,23 @@
2627
import java.util.List;
2728
import org.apache.hadoop.hbase.HBaseClassTestRule;
2829
import org.apache.hadoop.hbase.HBaseTestingUtility;
30+
import org.apache.hadoop.hbase.HRegionLocation;
31+
import org.apache.hadoop.hbase.MetaTableAccessor;
32+
import org.apache.hadoop.hbase.RegionLocations;
2933
import org.apache.hadoop.hbase.TableName;
3034
import org.apache.hadoop.hbase.testclassification.ClientTests;
3135
import org.apache.hadoop.hbase.testclassification.MediumTests;
3236
import org.apache.hadoop.hbase.util.Bytes;
37+
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
3338
import org.junit.AfterClass;
3439
import org.junit.BeforeClass;
3540
import org.junit.ClassRule;
41+
import org.junit.Rule;
3642
import org.junit.Test;
3743
import org.junit.experimental.categories.Category;
44+
import org.junit.rules.TestName;
45+
46+
import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
3847

3948
@Category({ MediumTests.class, ClientTests.class })
4049
public class TestRegionLocationCaching {
@@ -50,6 +59,9 @@ public class TestRegionLocationCaching {
5059
private static byte[] FAMILY = Bytes.toBytes("testFamily");
5160
private static byte[] QUALIFIER = Bytes.toBytes("testQualifier");
5261

62+
@Rule
63+
public final TestName name = new TestName();
64+
5365
@BeforeClass
5466
public static void setUpBeforeClass() throws Exception {
5567
TEST_UTIL.startMiniCluster(SLAVES);
@@ -62,6 +74,53 @@ public static void tearDownAfterClass() throws Exception {
6274
TEST_UTIL.shutdownMiniCluster();
6375
}
6476

77+
@Test
78+
public void testDoNotCacheLocationWithNullServerNameWhenGetAllLocations() throws Exception {
79+
TableName tableName = TableName.valueOf(name.getMethodName());
80+
TEST_UTIL.createTable(tableName, new byte[][] { FAMILY });
81+
TEST_UTIL.waitUntilAllRegionsAssigned(tableName);
82+
83+
ConnectionImplementation conn = (ConnectionImplementation) TEST_UTIL.getConnection();
84+
List<RegionInfo> regions = TEST_UTIL.getAdmin().getRegions(tableName);
85+
RegionInfo chosen = regions.get(0);
86+
87+
// re-populate region cache
88+
RegionLocator regionLocator = TEST_UTIL.getConnection().getRegionLocator(tableName);
89+
regionLocator.clearRegionLocationCache();
90+
regionLocator.getAllRegionLocations();
91+
92+
// expect all to be non-null at first
93+
checkRegions(tableName, conn, regions, null);
94+
95+
// clear servername from region info
96+
Put put = MetaTableAccessor.makePutFromRegionInfo(chosen, EnvironmentEdgeManager.currentTime());
97+
MetaTableAccessor.addEmptyLocation(put, 0);
98+
MetaTableAccessor.putsToMetaTable(TEST_UTIL.getConnection(), Lists.newArrayList(put));
99+
100+
// re-populate region cache again. check that we succeeded in nulling the servername
101+
regionLocator.clearRegionLocationCache();
102+
for (HRegionLocation loc : regionLocator.getAllRegionLocations()) {
103+
if (loc.getRegion().equals(chosen)) {
104+
assertNull(loc.getServerName());
105+
}
106+
}
107+
108+
// expect all but chosen to be non-null. chosen should be null because serverName was null
109+
checkRegions(tableName, conn, regions, chosen);
110+
}
111+
112+
private void checkRegions(TableName tableName, ConnectionImplementation conn,
113+
List<RegionInfo> regions, RegionInfo chosen) {
114+
for (RegionInfo region : regions) {
115+
RegionLocations fromCache = conn.getCachedLocation(tableName, region.getStartKey());
116+
if (region.equals(chosen)) {
117+
assertNull(fromCache);
118+
} else {
119+
assertNotNull(fromCache);
120+
}
121+
}
122+
}
123+
65124
@Test
66125
public void testCachingForHTableMultiplexerSinglePut() throws Exception {
67126
HTableMultiplexer multiplexer =

0 commit comments

Comments
 (0)