From 50f11151fdabcc4b328b96117db374bddefec40f Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Thu, 23 Jun 2022 08:24:49 -0400 Subject: [PATCH] HBASE-26790 getAllRegionLocations can cache locations with null hostname (#4565) Signed-off-by: Andrew Purtell --- .../client/AsyncTableRegionLocatorImpl.java | 8 ++- .../client/TestAsyncNonMetaRegionLocator.java | 49 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 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 fd04e662dd7..b7ec7fcd872 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 @@ -64,8 +64,12 @@ class AsyncTableRegionLocatorImpl implements AsyncTableRegionLocator { } CompletableFuture> future = ClientMetaTableAccessor .getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName); - addListener(future, (locs, error) -> locs - .forEach(loc -> conn.getLocator().getNonMetaRegionLocator().addLocationToCache(loc))); + 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; }, getClass().getSimpleName() + ".getAllRegionLocations"); } 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 f5f3cc4c6fb..6655bba3d4f 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; @@ -41,6 +42,7 @@ import org.apache.hadoop.hbase.CatalogReplicaMode; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtil; 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; @@ -52,6 +54,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; @@ -64,6 +67,7 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; import org.apache.hbase.thirdparty.com.google.common.io.Closeables; @Category({ MediumTests.class, ClientTests.class }) @@ -476,4 +480,49 @@ public class TestAsyncNonMetaRegionLocator { assertNotNull(conn.getLocator().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 + checkRegions(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().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 + checkRegions(conn, regions, chosen); + } + + private void checkRegions(AsyncConnectionImpl conn, List regions, RegionInfo chosen) { + for (RegionInfo region : regions) { + RegionLocations fromCache = + conn.getLocator().getRegionLocationInCache(TABLE_NAME, region.getStartKey()); + if (region.equals(chosen)) { + assertNull(fromCache); + } else { + assertNotNull(fromCache); + } + } + } }