HBASE-17350 Fixup of regionserver group-based assignment

Renamed move_rsgroup_servers as move_servers_rsgroup
Renamed move_rsgroup_tables as move_tables_rsgroup

Minor changes to help text in rsgroup commands making them all same.
Made LOG from RSGroupAdminServer all talk of 'rsgroup' rather than
'group' to be consistent.

Fix for table.jsp where it would fail to display regions because no
type for the protobuf record specified.

Fix it so that move of an offline server to 'default' rsgroup is like
moving the reference to the server to trash (keeps the 'default' group
consistently 'dynamic' regards its server-list).

Fixed another issue where we were stuck in a loop because regions
were in FAILED_OPEN state because no server to assign too so we'd
never recover (a vagary of the current state of Master assignement
but no less a possibility in real world deploys).

Make it so servers are sorted when we list them; its what operator
would expect.

hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java

@@ -20,16 +20,19 @@
 
 package org.apache.hadoop.hbase.rsgroup;
 
-import com.google.common.collect.Sets;
-import com.google.common.net.HostAndPort;
-
 import java.util.Collection;
 import java.util.NavigableSet;
 import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
 
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.hbase.classification.InterfaceStability;
+import org.apache.hadoop.hbase.util.Addressing;
+
+import com.google.common.collect.Sets;
+import com.google.common.net.HostAndPort;
 
 /**
  * Stores the group information of region server groups.
@@ -42,25 +45,24 @@ public class RSGroupInfo {
   public static final String NAMESPACEDESC_PROP_GROUP = "hbase.rsgroup.name";
 
   private String name;
-  private Set<HostAndPort> servers;
+  private SortedSet<HostAndPort> servers;
   private NavigableSet<TableName> tables;
 
   public RSGroupInfo(String name) {
-    this(name, Sets.<HostAndPort>newHashSet(), Sets.<TableName>newTreeSet());
+    this(name, Sets.<HostAndPort>newHashSet(), Sets.newTreeSet());
   }
 
   RSGroupInfo(String name,
               Set<HostAndPort> servers,
               NavigableSet<TableName> tables) {
     this.name = name;
-    this.servers = servers;
+    this.servers = new TreeSet<>(new Addressing.HostAndPortComparable());
-    this.tables = tables;
+    this.servers.addAll(servers);
+    this.tables = new TreeSet<>(tables);
   }
 
   public RSGroupInfo(RSGroupInfo src) {
-    name = src.getName();
+    this(src.getName(), src.servers, src.tables);
-    servers = Sets.newHashSet(src.getServers());
-    tables = Sets.newTreeSet(src.getTables());
   }
 
   /**

hbase-common/src/main/java/org/apache/hadoop/hbase/util/Addressing.java

@@ -24,10 +24,13 @@ import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.NetworkInterface;
 import java.net.SocketException;
+import java.util.Comparator;
 import java.util.Enumeration;
 
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 
+import com.google.common.net.HostAndPort;
+
 /**
  * Utility for network addresses, resolving and naming.
  */
@@ -36,6 +39,25 @@ public class Addressing {
   public static final String VALID_PORT_REGEX = "[\\d]+";
   public static final String HOSTNAME_PORT_SEPARATOR = ":";
 
+  /**
+   * HostAndPort Comparator.
+   * Does compare on HostAndPort instances. This comparator says that instances that have same
+   * host and port are the same. This is a little different than HostAndPort#equals. It does
+   * NOT consider two ipv6 HostAndPort instances the same if they have the same hostname
+   * and port and they differ only in the fact that one provided brackets around the ipv6
+   * hostname while the other did not: i.e. HostAndPort does NOT equate
+   * {@code HostAndPort.fromParts("[2001:db8::1]", 888);} and
+   * {@code HostAndPort.fromParts("2001:db8::1", 888);}.
+   */
+  public static class HostAndPortComparable implements Comparator<HostAndPort> {
+    @Override
+    public int compare(HostAndPort left, HostAndPort right) {
+      int compare = left.getHostText().compareTo(right.getHostText());
+      if (compare != 0) return compare;
+      return left.getPort() - right.getPort();
+    }
+  }
+
   /**
    * @param hostAndPort Formatted as <code>&lt;hostname&gt; ':' &lt;port&gt;</code>
    * @return An InetSocketInstance

hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestAddressing.java

@@ -0,0 +1,39 @@
+/*
+ * 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 regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.hadoop.hbase.util;
+
+import static org.junit.Assert.*;
+
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.hbase.util.Addressing.HostAndPortComparable;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import com.google.common.net.HostAndPort;
+
+@Category({MiscTests.class, SmallTests.class})
+public class TestAddressing {
+
+  @Test
+  public void testHostAndPortComparable() {
+    HostAndPortComparable c = new HostAndPortComparable();
+    HostAndPort left = HostAndPort.fromParts("[2001:db8::1]", 888);
+    HostAndPort right = HostAndPort.fromParts("2001:db8::1", 888);
+    assertTrue(left.toString() + " " + right.toString(), c.compare(left, right) == 0);
+  }
+}

hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java

@@ -19,23 +19,19 @@
  */
 package org.apache.hadoop.hbase.rsgroup;
 
-import com.google.common.collect.Lists;
+import static org.apache.hadoop.hbase.rsgroup.Utility.getOnlineServers;
-import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
-import com.google.common.net.HostAndPort;
 
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.NavigableMap;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentSkipListMap;
-import java.util.concurrent.ConcurrentMap;
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.commons.logging.Log;
@@ -55,6 +51,11 @@ import org.apache.hadoop.hbase.master.RegionState;
 import org.apache.hadoop.hbase.master.ServerManager;
 import org.apache.hadoop.hbase.master.locking.LockManager;
 import org.apache.hadoop.hbase.master.locking.LockProcedure;
+import org.apache.hadoop.hbase.util.Addressing;
+
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
+import com.google.common.net.HostAndPort;
 
 /**
  * Service to support Region Server Grouping (HBase-6721)
@@ -64,16 +65,16 @@ public class RSGroupAdminServer extends RSGroupAdmin {
   private static final Log LOG = LogFactory.getLog(RSGroupAdminServer.class);
 
   private MasterServices master;
-  //List of servers that are being moved from one group to another
+  // List of servers that are being moved from one group to another
-  //Key=host:port,Value=targetGroup
+  // Key=host:port,Value=targetGroup
-  private ConcurrentMap<HostAndPort,String> serversInTransition =
+  private NavigableMap<HostAndPort,String> serversInTransition =
-      new ConcurrentHashMap<HostAndPort, String>();
+      new ConcurrentSkipListMap<HostAndPort, String>(new Addressing.HostAndPortComparable());
-  private RSGroupInfoManager RSGroupInfoManager;
+  private RSGroupInfoManager rsgroupInfoManager;
 
   public RSGroupAdminServer(MasterServices master,
                             RSGroupInfoManager RSGroupInfoManager) throws IOException {
     this.master = master;
-    this.RSGroupInfoManager = RSGroupInfoManager;
+    this.rsgroupInfoManager = RSGroupInfoManager;
   }
 
   @Override
@@ -81,106 +82,96 @@ public class RSGroupAdminServer extends RSGroupAdmin {
     return getRSGroupInfoManager().getRSGroup(groupName);
   }
 
-
   @Override
   public RSGroupInfo getRSGroupInfoOfTable(TableName tableName) throws IOException {
     String groupName = getRSGroupInfoManager().getRSGroupOfTable(tableName);
-    if (groupName == null) {
+    return groupName == null? null: getRSGroupInfoManager().getRSGroup(groupName);
-      return null;
-    }
-    return getRSGroupInfoManager().getRSGroup(groupName);
   }
 
   @Override
   public void moveServers(Set<HostAndPort> servers, String targetGroupName)
       throws IOException {
     if (servers == null) {
-      throw new ConstraintException(
+      throw new ConstraintException("The list of servers to move cannot be null.");
-          "The list of servers cannot be null.");
     }
-    if (StringUtils.isEmpty(targetGroupName)) {
+    if (servers.isEmpty()) {
-      throw new ConstraintException("The target group cannot be null.");
-    }
-    if (servers.size() < 1) {
       return;
     }
-
+    if (StringUtils.isEmpty(targetGroupName)) {
+      throw new ConstraintException("The target rsgroup cannot be null.");
+    }
     RSGroupInfo targetGrp = getRSGroupInfo(targetGroupName);
     if (targetGrp == null) {
-      throw new ConstraintException("Group does not exist: "+targetGroupName);
+      throw new ConstraintException("RSGroup " + targetGroupName + " does not exist.");
     }
-
     RSGroupInfoManager manager = getRSGroupInfoManager();
     synchronized (manager) {
       if (master.getMasterCoprocessorHost() != null) {
         master.getMasterCoprocessorHost().preMoveServers(servers, targetGroupName);
       }
       HostAndPort firstServer = servers.iterator().next();
-      //we only allow a move from a single source group
+      // We only allow a move from a single source group so this should be ok
-      //so this should be ok
       RSGroupInfo srcGrp = manager.getRSGroupOfServer(firstServer);
-      //only move online servers (from default)
-      //or servers from other groups
-      //this prevents bogus servers from entering groups
       if (srcGrp == null) {
-        throw new ConstraintException(
+        throw new ConstraintException("Server " + firstServer + " does not have a rsgroup.");
-            "Server "+firstServer+" does not have a group.");
       }
+      if (srcGrp.getName().equals(targetGroupName)) {
+        throw new ConstraintException( "Target rsgroup " + targetGroupName +
+            " is same as source " + srcGrp + " rsgroup.");
+      }
+      // Only move online servers (from default) or servers from other groups.
+      // This prevents bogus servers from entering groups
       if (RSGroupInfo.DEFAULT_GROUP.equals(srcGrp.getName())) {
-        Set<HostAndPort> onlineServers = new HashSet<HostAndPort>();
+        Set<HostAndPort> onlineServers = getOnlineServers(this.master);
-        for(ServerName server: master.getServerManager().getOnlineServers().keySet()) {
+        for (HostAndPort el: servers) {
-          onlineServers.add(server.getHostPort());
+          if (!onlineServers.contains(el)) {
-        }
-        for(HostAndPort el: servers) {
-          if(!onlineServers.contains(el)) {
             throw new ConstraintException(
-                "Server "+el+" is not an online server in default group.");
+                "Server " + el + " is not an online server in 'default' rsgroup.");
           }
         }
       }
 
       if(srcGrp.getServers().size() <= servers.size() &&
           srcGrp.getTables().size() > 0) {
-        throw new ConstraintException("Cannot leave a group "+srcGrp.getName()+
+        throw new ConstraintException("Cannot leave a rsgroup " + srcGrp.getName() +
-            " that contains tables " +"without servers.");
+            " that contains tables without servers to host them.");
       }
 
-      String sourceGroupName = getRSGroupInfoManager()
+      String sourceGroupName =
-          .getRSGroupOfServer(srcGrp.getServers().iterator().next()).getName();
+          manager.getRSGroupOfServer(srcGrp.getServers().iterator().next()).getName();
-      if(getRSGroupInfo(targetGroupName) == null) {
+      if (getRSGroupInfo(targetGroupName) == null) {
-        throw new ConstraintException("Target group does not exist: "+targetGroupName);
+        throw new ConstraintException("Target " + targetGroupName + " rsgroup does not exist.");
       }
 
-      for(HostAndPort server: servers) {
+      for (HostAndPort server: servers) {
         if (serversInTransition.containsKey(server)) {
           throw new ConstraintException(
-              "Server list contains a server that is already being moved: "+server);
+              "Server list contains a server " + server + " that is already being moved.");
         }
-        String tmpGroup = getRSGroupInfoManager().getRSGroupOfServer(server).getName();
+        String tmpGroup = manager.getRSGroupOfServer(server).getName();
         if (sourceGroupName != null && !tmpGroup.equals(sourceGroupName)) {
           throw new ConstraintException(
-              "Move server request should only come from one source group. "+
+              "Move server request should only come from one source rsgroup. "+
-              "Expecting only "+sourceGroupName+" but contains "+tmpGroup);
+              "Expecting only " + sourceGroupName + " but contains " + tmpGroup);
         }
       }
 
-      if(sourceGroupName.equals(targetGroupName)) {
+      if (sourceGroupName.equals(targetGroupName)) {
         throw new ConstraintException(
-            "Target group is the same as source group: "+targetGroupName);
+            "Target rsgroup " + sourceGroupName + " is same as source rsgroup.");
       }
-
       try {
         //update the servers as in transition
         for (HostAndPort server : servers) {
           serversInTransition.put(server, targetGroupName);
         }
 
-        getRSGroupInfoManager().moveServers(servers, sourceGroupName, targetGroupName);
+        Set<HostAndPort> movedServers =
+            manager.moveServers(servers, sourceGroupName, targetGroupName);
         boolean found;
-        List<HostAndPort> tmpServers = Lists.newArrayList(servers);
         do {
           found = false;
-          for (Iterator<HostAndPort> iter = tmpServers.iterator();
+          for (Iterator<HostAndPort> iter = movedServers.iterator();
                iter.hasNext(); ) {
             HostAndPort rs = iter.next();
             //get online regions
@@ -208,6 +199,13 @@ public class RSGroupAdminServer extends RSGroupAdmin {
                 //so we need to filter
                 if (!targetGrp.containsTable(region.getTable())) {
                   master.getAssignmentManager().unassign(region);
+                  if (master.getAssignmentManager().getRegionStates().
+                      getRegionState(region).isFailedOpen()) {
+                    // If region is in FAILED_OPEN state, it won't recover, not without
+                    // operator intervention... in hbase-2.0.0 at least. Continue rather
+                    // than mark region as 'found'.
+                    continue;
+                  }
                   found = true;
                 }
               }
@@ -242,7 +240,7 @@ public class RSGroupAdminServer extends RSGroupAdmin {
       throw new ConstraintException(
           "The list of servers cannot be null.");
     }
-    if(tables.size() < 1) {
+    if (tables.size() < 1) {
       LOG.debug("moveTables() passed an empty set. Ignoring.");
       return;
     }
@@ -255,18 +253,19 @@ public class RSGroupAdminServer extends RSGroupAdmin {
       if(targetGroup != null) {
         RSGroupInfo destGroup = manager.getRSGroup(targetGroup);
         if(destGroup == null) {
-          throw new ConstraintException("Target group does not exist: "+targetGroup);
+          throw new ConstraintException("Target " + targetGroup + " rsgroup does not exist.");
         }
         if(destGroup.getServers().size() < 1) {
-          throw new ConstraintException("Target group must have at least one server.");
+          throw new ConstraintException("Target rsgroup must have at least one server.");
         }
       }
 
-      for(TableName table : tables) {
+      for (TableName table : tables) {
         String srcGroup = manager.getRSGroupOfTable(table);
         if(srcGroup != null && srcGroup.equals(targetGroup)) {
           throw new ConstraintException(
-              "Source group is the same as target group for table "+table+" :"+srcGroup);
+              "Source rsgroup " + srcGroup + " is same as target " + targetGroup +
+              " rsgroup for table " + table);
         }
       }
       manager.moveTables(tables, targetGroup);
@@ -276,7 +275,7 @@ public class RSGroupAdminServer extends RSGroupAdmin {
     }
     for (TableName table: tables) {
       LockManager.MasterLock lock = master.getLockManager().createMasterLock(table,
-          LockProcedure.LockType.EXCLUSIVE, this.getClass().getName() + ": Group: table move");
+          LockProcedure.LockType.EXCLUSIVE, this.getClass().getName() + ": RSGroup: table move");
       try {
         try {
           lock.acquire();
@@ -313,21 +312,25 @@ public class RSGroupAdminServer extends RSGroupAdmin {
       }
       RSGroupInfo RSGroupInfo = getRSGroupInfoManager().getRSGroup(name);
       if(RSGroupInfo == null) {
-        throw new ConstraintException("Group "+name+" does not exist");
+        throw new ConstraintException("RSGroup " + name + " does not exist");
       }
       int tableCount = RSGroupInfo.getTables().size();
       if (tableCount > 0) {
-        throw new ConstraintException("Group "+name+" must have no associated tables: "+tableCount);
+        throw new ConstraintException("RSGroup " + name + " has " + tableCount +
+            " tables; you must remove these tables from the rsgroup before " +
+            "the rsgroup can be removed.");
       }
       int serverCount = RSGroupInfo.getServers().size();
       if(serverCount > 0) {
-        throw new ConstraintException(
+        throw new ConstraintException("RSGroup " + name + " has " + serverCount +
-            "Group "+name+" must have no associated servers: "+serverCount);
+            " servers; you must remove these servers from the rsgroup before" +
+            "the rsgroup can be removed.");
       }
-      for(NamespaceDescriptor ns: master.getClusterSchema().getNamespaces()) {
+      for (NamespaceDescriptor ns: master.getClusterSchema().getNamespaces()) {
         String nsGroup = ns.getConfigurationValue(RSGroupInfo.NAMESPACEDESC_PROP_GROUP);
-        if(nsGroup != null &&  nsGroup.equals(name)) {
+        if (nsGroup != null &&  nsGroup.equals(name)) {
-          throw new ConstraintException("Group "+name+" is referenced by namespace: "+ns.getName());
+          throw new ConstraintException("RSGroup " + name + " is referenced by namespace: " +
+              ns.getName());
         }
       }
       manager.removeRSGroup(name);
@@ -349,7 +352,7 @@ public class RSGroupAdminServer extends RSGroupAdmin {
         master.getMasterCoprocessorHost().preBalanceRSGroup(groupName);
       }
       if (getRSGroupInfo(groupName) == null) {
-        throw new ConstraintException("Group does not exist: "+groupName);
+        throw new ConstraintException("RSGroup does not exist: "+groupName);
       }
       // Only allow one balance run at at time.
       Map<String, RegionState> groupRIT = rsGroupGetRegionsInTransition(groupName);
@@ -382,12 +385,12 @@ public class RSGroupAdminServer extends RSGroupAdmin {
       long startTime = System.currentTimeMillis();
       balancerRan = plans != null;
       if (plans != null && !plans.isEmpty()) {
-        LOG.info("Group balance "+groupName+" starting with plan count: "+plans.size());
+        LOG.info("RSGroup balance "+groupName+" starting with plan count: "+plans.size());
         for (RegionPlan plan: plans) {
           LOG.info("balance " + plan);
           assignmentManager.balance(plan);
         }
-        LOG.info("Group balance "+groupName+" completed after "+
+        LOG.info("RSGroup balance "+groupName+" completed after "+
             (System.currentTimeMillis()-startTime)+" seconds");
       }
       if (master.getMasterCoprocessorHost() != null) {
@@ -409,7 +412,7 @@ public class RSGroupAdminServer extends RSGroupAdmin {
 
   @InterfaceAudience.Private
   public RSGroupInfoManager getRSGroupInfoManager() throws IOException {
-    return RSGroupInfoManager;
+    return rsgroupInfoManager;
   }
 
   private Map<String, RegionState> rsGroupGetRegionsInTransition(String groupName)

hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManager.java

@@ -69,10 +69,10 @@ public interface RSGroupInfoManager {
    * @param hostPorts list of servers, must be part of the same group
    * @param srcGroup groupName being moved from
    * @param dstGroup groupName being moved to
-   * @return true if move was successful
+   * @return Set of servers moved (May be a subset of {@code hostPorts}).
    * @throws java.io.IOException on move failure
    */
-  boolean moveServers(Set<HostAndPort> hostPorts,
+  Set<HostAndPort> moveServers(Set<HostAndPort> hostPorts,
                       String srcGroup, String dstGroup) throws IOException;
 
   /**

hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java

@@ -84,6 +84,7 @@ import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.zookeeper.ZKUtil;
 import org.apache.hadoop.hbase.zookeeper.ZooKeeperWatcher;
 import org.apache.zookeeper.KeeperException;
+import static org.apache.hadoop.hbase.rsgroup.Utility.getOnlineServers;
 
 /**
  * This is an implementation of {@link RSGroupInfoManager}. Which makes
@@ -166,29 +167,42 @@ public class RSGroupInfoManagerImpl implements RSGroupInfoManager, ServerListene
   }
 
   @Override
-  public synchronized boolean moveServers(Set<HostAndPort> hostPorts, String srcGroup,
+  public synchronized Set<HostAndPort> moveServers(Set<HostAndPort> hostPorts,
-                                          String dstGroup) throws IOException {
+      String srcGroup, String dstGroup)
+  throws IOException {
     if (!rsGroupMap.containsKey(srcGroup)) {
-      throw new DoNotRetryIOException("Group "+srcGroup+" does not exist");
+      throw new DoNotRetryIOException("RSGroup " + srcGroup + " does not exist");
     }
     if (!rsGroupMap.containsKey(dstGroup)) {
-      throw new DoNotRetryIOException("Group "+dstGroup+" does not exist");
+      throw new DoNotRetryIOException("RSGroup " + dstGroup + " does not exist");
     }
-
     RSGroupInfo src = new RSGroupInfo(getRSGroup(srcGroup));
     RSGroupInfo dst = new RSGroupInfo(getRSGroup(dstGroup));
-    boolean foundOne = false;
+    // If destination is 'default' rsgroup, make sure servers is online.
-    for(HostAndPort el: hostPorts) {
+    // If not, just drop it.
-      foundOne = src.removeServer(el) || foundOne;
+    Set<HostAndPort> onlineServers = dst.getName().equals(RSGroupInfo.DEFAULT_GROUP)?
-      dst.addServer(el);
+        getOnlineServers(this.master): null;
+    Set<HostAndPort> result = new HashSet<>(hostPorts.size());
+    for (HostAndPort el: hostPorts) {
+      src.removeServer(el);
+      if (onlineServers != null) {
+        // onlineServers is non-null if 'default' rsgroup.
+        // If the server is not online, drop it.
+        if (!onlineServers.contains(el)) {
+          if (LOG.isDebugEnabled()) {
+            LOG.debug("Dropping " + el + " during move-to-default rsgroup because it is not online");
+          }
+          continue;
+        }
+      }
+      dst.addServer(el);
+      result.add(el);
     }
-
     Map<String,RSGroupInfo> newGroupMap = Maps.newHashMap(rsGroupMap);
     newGroupMap.put(src.getName(), src);
     newGroupMap.put(dst.getName(), dst);
-
     flushConfig(newGroupMap);
-    return foundOne;
+    return result;
   }
 
   /**
@@ -369,9 +383,7 @@ public class RSGroupInfoManagerImpl implements RSGroupInfoManager, ServerListene
     for(RSGroupInfo RSGroupInfo : newGroupMap.values()) {
       RSGroupProtos.RSGroupInfo proto = RSGroupSerDe.toProtoGroupInfo(RSGroupInfo);
       Put p = new Put(Bytes.toBytes(RSGroupInfo.getName()));
-      p.addColumn(META_FAMILY_BYTES,
+      p.addColumn(META_FAMILY_BYTES, META_QUALIFIER_BYTES, proto.toByteArray());
-          META_QUALIFIER_BYTES,
-          proto.toByteArray());
       mutations.add(p);
       for(TableName entry: RSGroupInfo.getTables()) {
         newTableMap.put(entry, RSGroupInfo.getName());
@@ -423,7 +435,7 @@ public class RSGroupInfoManagerImpl implements RSGroupInfoManager, ServerListene
       }
 
 
-      for(RSGroupInfo RSGroupInfo : newGroupMap.values()) {
+      for (RSGroupInfo RSGroupInfo : newGroupMap.values()) {
         String znode = ZKUtil.joinZNode(groupBasePath, RSGroupInfo.getName());
         RSGroupProtos.RSGroupInfo proto = RSGroupSerDe.toProtoGroupInfo(RSGroupInfo);
         LOG.debug("Updating znode: "+znode);

hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/Utility.java

@@ -0,0 +1,48 @@
+/**
+ * Copyright The Apache Software Foundation
+ *
+ * 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
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.rsgroup;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.classification.InterfaceAudience;
+import org.apache.hadoop.hbase.master.MasterServices;
+
+import com.google.common.net.HostAndPort;
+
+/**
+ * Utility for this RSGroup package in hbase-rsgroup.
+ */
+@InterfaceAudience.Private
+class Utility {
+  /**
+   * @param master
+   * @return Set of online Servers named for their hostname and port (not ServerName).
+   */
+  static Set<HostAndPort> getOnlineServers(final MasterServices master) {
+    Set<HostAndPort> onlineServers = new HashSet<HostAndPort>();
+    if (master == null) return onlineServers;
+    for(ServerName server: master.getServerManager().getOnlineServers().keySet()) {
+      onlineServers.add(server.getHostPort());
+    }
+    return onlineServers;
+  }
+}

hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsBase.java

@@ -279,7 +279,7 @@ public abstract class TestRSGroupsBase {
       rsGroupAdmin.moveServers(Sets.newHashSet(HostAndPort.fromString("foo:9999")),"foo");
       fail("Bogus servers shouldn't have been successfully moved.");
     } catch(IOException ex) {
-      String exp = "Server foo:9999 does not have a group.";
+      String exp = "Server foo:9999 does not have a rsgroup";
       String msg = "Expected '"+exp+"' in exception message: ";
       assertTrue(msg+" "+ex.getMessage(), ex.getMessage().contains(exp));
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java

@@ -618,7 +618,8 @@ public class MasterRpcServices extends RSRpcServices
       String methodName = call.getMethodName();
       if (!master.coprocessorServiceHandlers.containsKey(serviceName)) {
         throw new UnknownProtocolException(null,
-          "No registered master coprocessor service found for name "+serviceName);
+          "No registered Master Coprocessor Endpoint found for " + serviceName +
+          ". Has it been enabled?");
       }
 
       com.google.protobuf.Service service = master.coprocessorServiceHandlers.get(serviceName);

hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java

@@ -61,7 +61,6 @@ import org.apache.hadoop.hbase.monitoring.MonitoredTask;
 import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
 import org.apache.hadoop.hbase.regionserver.HRegionServer;
 import org.apache.hadoop.hbase.regionserver.RegionOpeningState;
-import org.apache.hadoop.hbase.security.User;
 import org.apache.hadoop.hbase.shaded.com.google.protobuf.ServiceException;
 import org.apache.hadoop.hbase.shaded.com.google.protobuf.UnsafeByteOperations;
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java

@@ -18,8 +18,6 @@
  */
 package org.apache.hadoop.hbase.regionserver;
 
-import com.google.common.annotations.VisibleForTesting;
-
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InterruptedIOException;
@@ -206,6 +204,7 @@ import org.apache.hadoop.hbase.wal.WALKey;
 import org.apache.hadoop.hbase.wal.WALSplitter;
 import org.apache.hadoop.hbase.zookeeper.ZKSplitLog;
 import org.apache.zookeeper.KeeperException;
+import com.google.common.annotations.VisibleForTesting;
 
 /**
  * Implements the regionserver RPC services.

hbase-server/src/main/resources/hbase-webapps/master/table.jsp

@@ -17,6 +17,7 @@
  * limitations under the License.
  */
 --%>
+<%@page import="org.apache.hadoop.hbase.protobuf.generated.HBaseProtos.RegionSpecifier.RegionSpecifierType"%>
 <%@ page contentType="text/html;charset=UTF-8"
   import="static org.apache.commons.lang.StringEscapeUtils.escapeXml"
   import="org.apache.hadoop.hbase.shaded.com.google.protobuf.ByteString"
@@ -48,6 +49,18 @@
   import="org.apache.hadoop.hbase.HBaseConfiguration"
   import="org.apache.hadoop.hbase.TableNotFoundException"%>
 <%@ page import="org.apache.hadoop.hbase.client.*" %>
+<%!
+  /**
+   * @return An empty region load stamped with the passed in <code>hri</code>
+   * region name.
+   */
+  private RegionLoad getEmptyRegionLoad(final HRegionInfo hri) {
+    return new RegionLoad(ClusterStatusProtos.RegionLoad.newBuilder().
+      setRegionSpecifier(HBaseProtos.RegionSpecifier.newBuilder().
+      setType(HBaseProtos.RegionSpecifier.RegionSpecifierType.REGION_NAME).
+      setValue(ByteString.copyFrom(hri.getRegionName())).build()).build());
+  }
+%>
 <%
   HMaster master = (HMaster)getServletContext().getAttribute(HMaster.MASTER);
   Configuration conf = master.getConfiguration();
@@ -370,15 +383,15 @@ if ( fqtn != null ) {
           totalMemSize += regionload.getMemStoreSizeMB();
           totalStoreFileSizeMB += regionload.getStorefileSizeMB();
         } else {
-          RegionLoad load0 = new RegionLoad(ClusterStatusProtos.RegionLoad.newBuilder().setRegionSpecifier(HBaseProtos.RegionSpecifier.newBuilder().setValue(ByteString.copyFrom(regionInfo.getRegionName())).build()).build());
+          RegionLoad load0 = getEmptyRegionLoad(regionInfo);
           regionsToLoad.put(regionInfo, load0);
         }
-      }else{
+      } else{
-        RegionLoad load0 = new RegionLoad(ClusterStatusProtos.RegionLoad.newBuilder().setRegionSpecifier(HBaseProtos.RegionSpecifier.newBuilder().setValue(ByteString.copyFrom(regionInfo.getRegionName())).build()).build());
+        RegionLoad load0 = getEmptyRegionLoad(regionInfo);
         regionsToLoad.put(regionInfo, load0);
       }
-    }else{
+    } else {
-      RegionLoad load0 = new RegionLoad(ClusterStatusProtos.RegionLoad.newBuilder().setRegionSpecifier(HBaseProtos.RegionSpecifier.newBuilder().setValue(ByteString.copyFrom(regionInfo.getRegionName())).build()).build());
+      RegionLoad load0 = getEmptyRegionLoad(regionInfo);
       regionsToLoad.put(regionInfo, load0);
     }
   }
@@ -650,7 +663,7 @@ ShowDetailName&Start/End Key<input type="checkbox" id="showWhole" style="margin-
   %>
 </tr>
 <% } %>
-+<% } %>
+<% } %>
 </table>
 <% if (numRegions > numRegionsRendered) {
      String allRegionsUrl = "?name=" + fqtn + "&numRegions=all";

hbase-shell/src/main/ruby/shell.rb

@@ -460,15 +460,16 @@ Shell.load_command_group(
 Shell.load_command_group(
   'rsgroup',
   :full_name => 'RSGroups',
-  :comment => "NOTE: Above commands are only applicable if running with the Groups setup",
+  :comment => "NOTE: The rsgroup Coprocessor Endpoint must be enabled on the Master else commands fail with:
+  UnknownProtocolException: No registered Master Coprocessor Endpoint found for RSGroupAdminService",
   :commands => %w[
     list_rsgroups
     get_rsgroup
     add_rsgroup
     remove_rsgroup
     balance_rsgroup
-    move_rsgroup_servers
+    move_servers_rsgroup
-    move_rsgroup_tables
+    move_tables_rsgroup
     get_server_rsgroup
     get_table_rsgroup
   ]

hbase-shell/src/main/ruby/shell/commands.rb

@@ -47,7 +47,6 @@ module Shell
           puts "ERROR: #{rootCause}"
           puts "Backtrace: #{rootCause.backtrace.join("\n           ")}" if debug
           puts
-          puts "Here is some help for this command:"
           puts help
           puts
         else

hbase-shell/src/main/ruby/shell/commands/add_rsgroup.rb

@@ -23,11 +23,12 @@ module Shell
     class AddRsgroup < Command
       def help
         return <<-EOF
-Create a new region server group.
+Create a new RegionServer group.
 
 Example:
 
   hbase> add_rsgroup 'my_group'
+
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/balance_rsgroup.rb

@@ -23,9 +23,12 @@ module Shell
     class BalanceRsgroup < Command
       def help
         return <<-EOF
-Balance a region server group
+Balance a RegionServer group
+
+Example:
 
   hbase> balance_rsgroup 'my_group'
+
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/get_rsgroup.rb

@@ -23,16 +23,17 @@ module Shell
     class GetRsgroup < Command
       def help
         return <<-EOF
-Get a region server group's information.
+Get a RegionServer group's information.
 
 Example:
 
   hbase> get_rsgroup 'default'
+
 EOF
       end
 
       def command(group_name)
-        formatter.header(['GROUP INFORMATION'])
+        formatter.header(['RSGROUP '.concat(group_name)])
         rsgroup_admin.get_rsgroup(group_name) do |s|
           formatter.row([s])
         end

hbase-shell/src/main/ruby/shell/commands/get_server_rsgroup.rb

@@ -23,9 +23,12 @@ module Shell
     class GetServerRsgroup < Command
       def help
         return <<-EOF
-Get the group name the given region server is a member of.
+Get the group name the given RegionServer is a member of.
+
+Example:
 
   hbase> get_server_rsgroup 'server1:port1'
+
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/get_table_rsgroup.rb

@@ -23,9 +23,12 @@ module Shell
     class GetTableRsgroup < Command
       def help
         return <<-EOF
-Get the group name the given table is a member of.
+Get the RegionServer group name the given table is a member of.
+
+Example:
 
   hbase> get_table_rsgroup 'myTable'
+
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/list_procedures.rb

@@ -22,7 +22,7 @@ module Shell
     class ListProcedures < Command
       def help
         return <<-EOF
-List all procedures in hbase. Examples:
+List all procedures in hbase. For example:
 
   hbase> list_procedures
 EOF

hbase-shell/src/main/ruby/shell/commands/list_rsgroups.rb

@@ -23,13 +23,14 @@ module Shell
     class ListRsgroups < Command
       def help
         return <<-EOF
-List all region server groups. Optional regular expression parameter could
+List all RegionServer groups. Optional regular expression parameter can
 be used to filter the output.
 
 Example:
 
   hbase> list_rsgroups
   hbase> list_rsgroups 'abc.*'
+
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/move_servers_rsgroup.rb

@@ -20,12 +20,15 @@
 
 module Shell
   module Commands
-    class MoveRsgroupServers < Command
+    class MoveServersRsgroup < Command
       def help
         return <<-EOF
-Reassign a region server from one group to another.
+Reassign RegionServers from one group to another.
+
+Example:
+
+  hbase> move_servers_rsgroup 'dest',['server1:port','server2:port']
 
-  hbase> move_rsgroup_servers 'dest',['server1:port','server2:port']
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/move_tables_rsgroup.rb

@@ -20,12 +20,15 @@
 
 module Shell
   module Commands
-    class MoveRsgroupTables < Command
+    class MoveTablesRsgroup < Command
       def help
         return <<-EOF
-Reassign tables from one group to another.
+Reassign tables from one RegionServer group to another.
+
+Example:
+
+  hbase> move_tables_rsgroup 'dest',['table1','table2']
 
-  hbase> move_rsgroup_tables 'dest',['table1','table2']
 EOF
       end
 

hbase-shell/src/main/ruby/shell/commands/remove_rsgroup.rb

@@ -23,9 +23,10 @@ module Shell
     class RemoveRsgroup < Command
       def help
         return <<-EOF
-Remove a group.
+Remove a RegionServer group.
 
   hbase> remove_rsgroup 'my_group'
+
 EOF
       end
 

hbase-shell/src/test/ruby/shell/rsgroup_shell_test.rb

@@ -51,13 +51,13 @@ module Hbase
       @shell.command('get_rsgroup', 'default')
       hostPortStr = hostport.toString
       @shell.command('get_server_rsgroup', [hostPortStr])
-      @shell.command('move_rsgroup_servers',
+      @shell.command('move_servers_rsgroup',
                      group_name,
                      [hostPortStr])
       assert_equal(1, @rsgroup_admin.getRSGroupInfo(group_name).getServers.count)
       assert_equal(group_name, @rsgroup_admin.getRSGroupOfServer(hostport).getName)
 
-      @shell.command('move_rsgroup_tables',
+      @shell.command('move_tables_rsgroup',
                      group_name,
                      [table_name])
       assert_equal(1, @rsgroup_admin.getRSGroupInfo(group_name).getTables.count)