HBASE-18356 Enable TestFavoredStochasticBalancerPickers#testPickers that was disabled by Proc-V2 AM in HBASE-14614

Rebase/Fixup

Signed-off-by: Thiruvel Thirumoolan <thiruvel at yahoo-inc dot com>
Signed-off-by: Michael Stack <stack@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodesManager.java

@@ -46,6 +46,7 @@ import org.apache.hadoop.hdfs.HdfsConfiguration;
 import org.apache.hadoop.net.NetUtils;
 import org.apache.yetus.audience.InterfaceAudience;
 
+import org.apache.hadoop.hbase.shaded.com.google.common.annotations.VisibleForTesting;
 import org.apache.hadoop.hbase.shaded.com.google.common.collect.Lists;
 import org.apache.hadoop.hbase.shaded.com.google.common.collect.Maps;
 import org.apache.hadoop.hbase.shaded.com.google.common.collect.Sets;
@@ -287,6 +288,23 @@ public class FavoredNodesManager {
     }
   }
 
+  @VisibleForTesting
+  public synchronized Set<RegionInfo> getRegionsOfFavoredNode(ServerName serverName) {
+    Set<RegionInfo> regionInfos = Sets.newHashSet();
+
+    ServerName serverToUse = ServerName.valueOf(serverName.getHostAndPort(), NON_STARTCODE);
+    if (primaryRSToRegionMap.containsKey(serverToUse)) {
+      regionInfos.addAll(primaryRSToRegionMap.get(serverToUse));
+    }
+    if (secondaryRSToRegionMap.containsKey(serverToUse)) {
+      regionInfos.addAll(secondaryRSToRegionMap.get(serverToUse));
+    }
+    if (teritiaryRSToRegionMap.containsKey(serverToUse)) {
+      regionInfos.addAll(teritiaryRSToRegionMap.get(serverToUse));
+    }
+    return regionInfos;
+  }
+
   public RackManager getRackManager() {
     return rackManager;
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/favored/TestFavoredNodeAssignmentHelper.java

@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information

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

@@ -54,13 +54,11 @@ public class TestMasterMetrics {
     public MyMaster(Configuration conf) throws IOException, KeeperException, InterruptedException {
       super(conf);
     }
-/*
     @Override
     protected void tryRegionServerReport(
         long reportStartTime, long reportEndTime) {
       // do nothing
     }
-*/
   }
 
   @BeforeClass

hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestFavoredStochasticBalancerPickers.java

@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -23,7 +23,6 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.List;
@@ -35,26 +34,36 @@ import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.ClusterStatus;
 import org.apache.hadoop.hbase.ClusterStatus.Option;
 import org.apache.hadoop.hbase.HBaseTestingUtility;
-import org.apache.hadoop.hbase.HColumnDescriptor;
 import org.apache.hadoop.hbase.HConstants;
-import org.apache.hadoop.hbase.HTableDescriptor;
+import org.apache.hadoop.hbase.MiniHBaseCluster;
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.Waiter;
 import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
 import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
 import org.apache.hadoop.hbase.favored.FavoredNodesManager;
+import org.apache.hadoop.hbase.master.HMaster;
 import org.apache.hadoop.hbase.master.LoadBalancer;
 import org.apache.hadoop.hbase.master.RackManager;
+import org.apache.hadoop.hbase.master.assignment.RegionStates;
 import org.apache.hadoop.hbase.master.balancer.BaseLoadBalancer.Cluster;
+import org.apache.hadoop.hbase.regionserver.HRegion;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.regionserver.Region;
 import org.apache.hadoop.hbase.testclassification.LargeTests;
 import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.util.JVMClusterUtil;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestName;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.BeforeClass;
-import org.junit.Ignore;
+import org.junit.Rule;
 import org.junit.Test;
-import org.junit.experimental.categories.Category;
 
 import org.apache.hadoop.hbase.shaded.com.google.common.collect.Lists;
 import org.apache.hadoop.hbase.shaded.com.google.common.collect.Maps;
@@ -70,6 +79,10 @@ public class TestFavoredStochasticBalancerPickers extends BalancerTestBase {
   private static Configuration conf;
 
   private Admin admin;
+  private MiniHBaseCluster cluster;
+
+  @Rule
+  public TestName name = new TestName();
 
   @BeforeClass
   public static void setupBeforeClass() throws Exception {
@@ -88,6 +101,7 @@ public class TestFavoredStochasticBalancerPickers extends BalancerTestBase {
     TEST_UTIL.startMiniCluster(SLAVES);
     TEST_UTIL.getDFSCluster().waitClusterUp();
     TEST_UTIL.getHBaseCluster().waitForActiveAndReadyMaster(120*1000);
+    cluster = TEST_UTIL.getMiniHBaseCluster();
     admin = TEST_UTIL.getAdmin();
     admin.setBalancerRunning(false, true);
   }
@@ -99,34 +113,56 @@ public class TestFavoredStochasticBalancerPickers extends BalancerTestBase {
   }
 
 
-  @Ignore @Test
+  @Test
   public void testPickers() throws Exception {
-
+    TableName tableName = TableName.valueOf(name.getMethodName());
-    TableName tableName = TableName.valueOf("testPickers");
+    ColumnFamilyDescriptor columnFamilyDescriptor =
-    HTableDescriptor desc = new HTableDescriptor(tableName);
+        ColumnFamilyDescriptorBuilder.newBuilder(HConstants.CATALOG_FAMILY).build();
-    desc.addFamily(new HColumnDescriptor(HConstants.CATALOG_FAMILY));
+    TableDescriptor desc = TableDescriptorBuilder
+        .newBuilder(tableName)
+        .addColumnFamily(columnFamilyDescriptor)
+        .build();
     admin.createTable(desc, Bytes.toBytes("aaa"), Bytes.toBytes("zzz"), REGIONS);
+    TEST_UTIL.waitUntilAllRegionsAssigned(tableName);
     TEST_UTIL.loadTable(admin.getConnection().getTable(tableName), HConstants.CATALOG_FAMILY);
     admin.flush(tableName);
 
-    ServerName masterServerName = TEST_UTIL.getMiniHBaseCluster().getServerHoldingMeta();
+    HMaster master = cluster.getMaster();
-    final ServerName mostLoadedServer = getRSWithMaxRegions(Lists.newArrayList(masterServerName));
+    FavoredNodesManager fnm = master.getFavoredNodesManager();
+    ServerName masterServerName = master.getServerName();
+    List<ServerName> excludedServers = Lists.newArrayList(masterServerName);
+    final ServerName mostLoadedServer = getRSWithMaxRegions(tableName, excludedServers);
     assertNotNull(mostLoadedServer);
-    int numRegions = admin.getOnlineRegions(mostLoadedServer).size();
+    int numRegions = getTableRegionsFromServer(tableName, mostLoadedServer).size();
-    ServerName source = getRSWithMaxRegions(Lists.newArrayList(masterServerName, mostLoadedServer));
+    excludedServers.add(mostLoadedServer);
+    // Lets find another server with more regions to calculate number of regions to move
+    ServerName source = getRSWithMaxRegions(tableName, excludedServers);
     assertNotNull(source);
-    int regionsToMove = admin.getOnlineRegions(source).size()/2;
+    int regionsToMove = getTableRegionsFromServer(tableName, source).size()/2;
-    List<RegionInfo> hris = admin.getRegions(source);
+
+    // Since move only works if the target is part of favored nodes of the region, lets get all
+    // regions that are movable to mostLoadedServer
+    List<RegionInfo> hris = getRegionsThatCanBeMoved(tableName, mostLoadedServer);
+    RegionStates rst = master.getAssignmentManager().getRegionStates();
     for (int i = 0; i < regionsToMove; i++) {
-      admin.move(hris.get(i).getEncodedNameAsBytes(), Bytes.toBytes(mostLoadedServer.getServerName()));
+      final RegionInfo regionInfo = hris.get(i);
+      admin.move(regionInfo.getEncodedNameAsBytes(),
+          Bytes.toBytes(mostLoadedServer.getServerName()));
       LOG.info("Moving region: " + hris.get(i).getRegionNameAsString() + " to " + mostLoadedServer);
+      TEST_UTIL.waitFor(60000, new Waiter.Predicate<Exception>() {
+        @Override
+        public boolean evaluate() throws Exception {
+          return ServerName.isSameAddress(
+              rst.getRegionServerOfRegion(regionInfo), mostLoadedServer);
+        }
+      });
     }
     final int finalRegions = numRegions + regionsToMove;
     TEST_UTIL.waitUntilNoRegionsInTransition(60000);
     TEST_UTIL.waitFor(60000, new Waiter.Predicate<Exception>() {
       @Override
       public boolean evaluate() throws Exception {
-        int numRegions = TEST_UTIL.getAdmin().getOnlineRegions(mostLoadedServer).size();
+        int numRegions = getTableRegionsFromServer(tableName, mostLoadedServer).size();
         return (numRegions == finalRegions);
       }
     });
@@ -136,7 +172,7 @@ public class TestFavoredStochasticBalancerPickers extends BalancerTestBase {
     ClusterStatus status = admin.getClusterStatus(EnumSet.of(Option.LIVE_SERVERS));
     for (ServerName sn : status.getServers()) {
       if (!ServerName.isSameAddress(sn, masterServerName)) {
-        serverAssignments.put(sn, admin.getRegions(sn));
+        serverAssignments.put(sn, getTableRegionsFromServer(tableName, sn));
       }
     }
     RegionLocationFinder regionFinder = new RegionLocationFinder();
@@ -146,7 +182,7 @@ public class TestFavoredStochasticBalancerPickers extends BalancerTestBase {
     Cluster cluster = new Cluster(serverAssignments, null, regionFinder, new RackManager(conf));
     LoadOnlyFavoredStochasticBalancer balancer = (LoadOnlyFavoredStochasticBalancer) TEST_UTIL
         .getMiniHBaseCluster().getMaster().getLoadBalancer();
-    FavoredNodesManager fnm = TEST_UTIL.getMiniHBaseCluster().getMaster().getFavoredNodesManager();
+
     cluster.sortServersByRegionCount();
     Integer[] servers = cluster.serverIndicesSortedByRegionCount;
     LOG.info("Servers sorted by region count:" + Arrays.toString(servers));
@@ -180,22 +216,55 @@ public class TestFavoredStochasticBalancerPickers extends BalancerTestBase {
     assertTrue("load picker did not pick expected regions in 100 iterations.", userRegionPicked);
   }
 
-  private ServerName getRSWithMaxRegions(ArrayList<ServerName> excludeNodes) throws IOException {
+  /*
+   * A region can only be moved to one of its favored node. Hence this method helps us to
+   * get that list which makes it easy to write non-flaky tests.
+   */
+  private List<RegionInfo> getRegionsThatCanBeMoved(TableName tableName,
+      ServerName serverName) {
+    List<RegionInfo> regions = Lists.newArrayList();
+    RegionStates rst = cluster.getMaster().getAssignmentManager().getRegionStates();
+    FavoredNodesManager fnm = cluster.getMaster().getFavoredNodesManager();
+    for (RegionInfo regionInfo : fnm.getRegionsOfFavoredNode(serverName)) {
+      if (regionInfo.getTable().equals(tableName) &&
+          !ServerName.isSameAddress(rst.getRegionServerOfRegion(regionInfo), serverName)) {
+        regions.add(regionInfo);
+      }
+    }
+    return regions;
+  }
+
+  private List<RegionInfo> getTableRegionsFromServer(TableName tableName, ServerName source)
+      throws IOException {
+    List<RegionInfo> regionInfos = Lists.newArrayList();
+    HRegionServer regionServer = cluster.getRegionServer(source);
+    for (Region region : regionServer.getRegions(tableName)) {
+      regionInfos.add(region.getRegionInfo());
+    }
+    return regionInfos;
+  }
+
+  private ServerName getRSWithMaxRegions(TableName tableName, List<ServerName> excludeNodes)
+      throws IOException {
+
     int maxRegions = 0;
     ServerName maxLoadedServer = null;
 
-    for (ServerName sn : admin.getClusterStatus(EnumSet.of(Option.LIVE_SERVERS)).getServers()) {
+    for (JVMClusterUtil.RegionServerThread rst : cluster.getLiveRegionServerThreads()) {
-      if (admin.getOnlineRegions(sn).size() > maxRegions) {
+      List<HRegion> regions = rst.getRegionServer().getRegions(tableName);
-        if (excludeNodes == null || !doesMatchExcludeNodes(excludeNodes, sn)) {
+      LOG.debug("Server: " + rst.getRegionServer().getServerName() + " regions: " + regions.size());
-          maxRegions = admin.getOnlineRegions(sn).size();
+      if (regions.size() > maxRegions) {
-          maxLoadedServer = sn;
+        if (excludeNodes == null ||
+            !doesMatchExcludeNodes(excludeNodes, rst.getRegionServer().getServerName())) {
+          maxRegions = regions.size();
+          maxLoadedServer = rst.getRegionServer().getServerName();
         }
       }
     }
     return maxLoadedServer;
   }
 
-  private boolean doesMatchExcludeNodes(ArrayList<ServerName> excludeNodes, ServerName sn) {
+  private boolean doesMatchExcludeNodes(List<ServerName> excludeNodes, ServerName sn) {
     for (ServerName excludeSN : excludeNodes) {
       if (ServerName.isSameAddress(sn, excludeSN)) {
         return true;