HBASE-26790 getAllRegionLocations can cache locations with null hostname (#4575)

Signed-off-by: Andrew Purtell <apurtell@apache.org>

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

@@ -64,8 +64,12 @@ class AsyncTableRegionLocatorImpl implements AsyncTableRegionLocator {
       }
       CompletableFuture<List<HRegionLocation>> future = AsyncMetaTableAccessor
         .getTableHRegionLocations(conn.getTable(TableName.META_TABLE_NAME), tableName);
-      addListener(future, (locs, error) -> locs
+      addListener(future, (locs, error) -> locs.forEach(loc -> {
-        .forEach(loc -> conn.getLocator().getNonMetaRegionLocator().addLocationToCache(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");
   }

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

@@ -101,7 +101,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;
     }, HRegionLocator::getRegionNames, supplier);

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.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;
@@ -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,66 @@ public class TestAsyncNonMetaRegionLocator {
         region.getStartKey()));
     }
   }
+
+  @Test
+  public void testDoNotCacheLocationWithNullServerNameWhenGetAllLocations() throws Exception {
+    createMultiRegionTable();
+    AsyncConnectionImpl conn = (AsyncConnectionImpl) ConnectionFactory
+      .createAsyncConnection(TEST_UTIL.getConfiguration()).get();
+    List<RegionInfo> 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<RegionInfo> 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<RegionInfo> 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);
+      }
+    }
+  }
 }

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

@@ -18,6 +18,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;
 
@@ -26,15 +27,23 @@ import java.util.ArrayList;
 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 {
@@ -50,6 +59,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);
@@ -62,6 +74,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<RegionInfo> 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<RegionInfo> 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 =