From 3c3927c60f0c87923cbef19f985bad4cf7cc5e07 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Fri, 24 Jun 2022 07:40:42 -0400 Subject: [PATCH] HBASE-26790 getAllRegionLocations can cache locations with null hostname (#4575) Signed-off-by: Andrew Purtell --- .../client/AsyncTableRegionLocatorImpl.java | 10 ++- .../hadoop/hbase/client/HRegionLocator.java | 6 +- .../client/TestAsyncNonMetaRegionLocator.java | 67 +++++++++++++++++++ .../client/TestRegionLocationCaching.java | 59 ++++++++++++++++ 4 files changed, 138 insertions(+), 4 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java index da777199d61..d275336f51b 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableRegionLocatorImpl.java @@ -61,9 +61,13 @@ class AsyncTableRegionLocatorImpl implements AsyncTableRegionLocator { .thenApply(locs -> Arrays.asList(locs.getRegionLocations())); } CompletableFuture> future = AsyncMetaTableAccessor - .getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName); - addListener(future, (locs, error) -> locs.forEach(loc -> conn.getLocator() - .getNonMetaRegionLocator().addLocationToCache(loc))); + .getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName); + addListener(future, (locs, error) -> locs.forEach(loc -> { + // the cache assumes that all locations have a serverName. only add if that's true + if (loc.getServerName() != null) { + conn.getLocator().getNonMetaRegionLocator().addLocationToCache(loc); + } + })); return future; } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java index 8806a3e3f28..bd58a9000c7 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java @@ -79,7 +79,11 @@ public class HRegionLocator implements RegionLocator { for (HRegionLocation location : locations.getRegionLocations()) { regions.add(location); } - connection.cacheLocation(tableName, locations); + RegionLocations cleaned = locations.removeElementsWithNullLocation(); + // above can return null if all locations had null location + if (cleaned != null) { + connection.cacheLocation(tableName, cleaned); + } } return regions; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncNonMetaRegionLocator.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncNonMetaRegionLocator.java index 9ec1f963055..97b9d9d9afc 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncNonMetaRegionLocator.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncNonMetaRegionLocator.java @@ -26,6 +26,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import java.io.IOException; @@ -42,6 +43,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.NotServingRegionException; import org.apache.hadoop.hbase.RegionLocations; import org.apache.hadoop.hbase.ServerName; @@ -53,6 +55,7 @@ import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.ServerRegionReplicaUtil; import org.junit.After; import org.junit.AfterClass; @@ -65,6 +68,8 @@ import org.junit.runners.Parameterized; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; + @Category({ MediumTests.class, ClientTests.class }) @RunWith(Parameterized.class) public class TestAsyncNonMetaRegionLocator { @@ -480,4 +485,66 @@ public class TestAsyncNonMetaRegionLocator { .getRegionLocationInCache(TABLE_NAME, region.getStartKey())); } } + + @Test + public void testDoNotCacheLocationWithNullServerNameWhenGetAllLocations() throws Exception { + createMultiRegionTable(); + AsyncConnectionImpl conn = (AsyncConnectionImpl) ConnectionFactory + .createAsyncConnection(TEST_UTIL.getConfiguration()).get(); + List regions = TEST_UTIL.getAdmin().getRegions(TABLE_NAME); + RegionInfo chosen = regions.get(0); + + // re-populate region cache + AsyncTableRegionLocator regionLocator = conn.getRegionLocator(TABLE_NAME); + regionLocator.clearRegionLocationCache(); + regionLocator.getAllRegionLocations().get(); + + // expect all to be non-null at first + int tries = 3; + checkRegionsWithRetries(conn, regions, null, tries); + + // clear servername from region info + Put put = MetaTableAccessor.makePutFromRegionInfo(chosen, EnvironmentEdgeManager.currentTime()); + MetaTableAccessor.addEmptyLocation(put, 0); + MetaTableAccessor.putsToMetaTable(TEST_UTIL.getConnection(), Lists.newArrayList(put)); + + // re-populate region cache again. check that we succeeded in nulling the servername + regionLocator.clearRegionLocationCache(); + for (HRegionLocation loc : regionLocator.getAllRegionLocations().get()) { + if (loc.getRegion().equals(chosen)) { + assertNull(loc.getServerName()); + } + } + + // expect all but chosen to be non-null. chosen should be null because serverName was null + checkRegionsWithRetries(conn, regions, chosen, tries); + } + + // caching of getAllRegionLocations is async. so we give it a couple tries + private void checkRegionsWithRetries(AsyncConnectionImpl conn, List regions, + RegionInfo chosen, int retries) throws InterruptedException { + while (true) { + try { + checkRegions(conn, regions, chosen); + break; + } catch (AssertionError e) { + if (retries-- <= 0) { + throw e; + } + Thread.sleep(500); + } + } + } + + private void checkRegions(AsyncConnectionImpl conn, List regions, RegionInfo chosen) { + for (RegionInfo region : regions) { + RegionLocations fromCache = conn.getLocator().getNonMetaRegionLocator() + .getRegionLocationInCache(TABLE_NAME, region.getStartKey()); + if (region.equals(chosen)) { + assertNull(fromCache); + } else { + assertNotNull(fromCache); + } + } + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRegionLocationCaching.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRegionLocationCaching.java index 0134276acb2..035c13d75dc 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRegionLocationCaching.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRegionLocationCaching.java @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.client; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -28,15 +29,23 @@ import java.util.List; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.MetaTableAccessor; +import org.apache.hadoop.hbase.RegionLocations; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.ClassRule; +import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; + +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; @Category({MediumTests.class, ClientTests.class}) public class TestRegionLocationCaching { @@ -52,6 +61,9 @@ public class TestRegionLocationCaching { private static byte[] FAMILY = Bytes.toBytes("testFamily"); private static byte[] QUALIFIER = Bytes.toBytes("testQualifier"); + @Rule + public final TestName name = new TestName(); + @BeforeClass public static void setUpBeforeClass() throws Exception { TEST_UTIL.startMiniCluster(SLAVES); @@ -64,6 +76,53 @@ public class TestRegionLocationCaching { TEST_UTIL.shutdownMiniCluster(); } + @Test + public void testDoNotCacheLocationWithNullServerNameWhenGetAllLocations() throws Exception { + TableName tableName = TableName.valueOf(name.getMethodName()); + TEST_UTIL.createTable(tableName, new byte[][] { FAMILY }); + TEST_UTIL.waitUntilAllRegionsAssigned(tableName); + + ConnectionImplementation conn = (ConnectionImplementation) TEST_UTIL.getConnection(); + List regions = TEST_UTIL.getAdmin().getRegions(tableName); + RegionInfo chosen = regions.get(0); + + // re-populate region cache + RegionLocator regionLocator = TEST_UTIL.getConnection().getRegionLocator(tableName); + regionLocator.clearRegionLocationCache(); + regionLocator.getAllRegionLocations(); + + // expect all to be non-null at first + checkRegions(tableName, conn, regions, null); + + // clear servername from region info + Put put = MetaTableAccessor.makePutFromRegionInfo(chosen, EnvironmentEdgeManager.currentTime()); + MetaTableAccessor.addEmptyLocation(put, 0); + MetaTableAccessor.putsToMetaTable(TEST_UTIL.getConnection(), Lists.newArrayList(put)); + + // re-populate region cache again. check that we succeeded in nulling the servername + regionLocator.clearRegionLocationCache(); + for (HRegionLocation loc : regionLocator.getAllRegionLocations()) { + if (loc.getRegion().equals(chosen)) { + assertNull(loc.getServerName()); + } + } + + // expect all but chosen to be non-null. chosen should be null because serverName was null + checkRegions(tableName, conn, regions, chosen); + } + + private void checkRegions(TableName tableName, ConnectionImplementation conn, + List regions, RegionInfo chosen) { + for (RegionInfo region : regions) { + RegionLocations fromCache = conn.getCachedLocation(tableName, region.getStartKey()); + if (region.equals(chosen)) { + assertNull(fromCache); + } else { + assertNotNull(fromCache); + } + } + } + @Test public void testCachingForHTableMultiplexerSinglePut() throws Exception { HTableMultiplexer multiplexer =