Apache9 commented on a change in pull request #426: HBASE-22695 Store the rsgroup of a table in table configuration URL: https://github.com/apache/hbase/pull/426#discussion_r308626014
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java ########## @@ -110,117 +95,91 @@ RSGroupAdminServiceImpl getGroupAdminService() { return groupAdminService; } - private void assignTableToGroup(TableDescriptor desc) throws IOException { - String groupName = - master.getClusterSchema().getNamespace(desc.getTableName().getNamespaceAsString()) - .getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP); - if (groupName == null) { - groupName = RSGroupInfo.DEFAULT_GROUP; - } - RSGroupInfo rsGroupInfo = groupAdminServer.getRSGroupInfo(groupName); - if (rsGroupInfo == null) { - throw new ConstraintException( - "Default RSGroup (" + groupName + ") for this table's namespace does not exist."); - } - if (!rsGroupInfo.containsTable(desc.getTableName())) { - LOG.debug("Pre-moving table " + desc.getTableName() + " to RSGroup " + groupName); - groupAdminServer.moveTables(Sets.newHashSet(desc.getTableName()), groupName); - } - } - ///////////////////////////////////////////////////////////////////////////// // MasterObserver overrides ///////////////////////////////////////////////////////////////////////////// - private boolean rsgroupHasServersOnline(TableDescriptor desc) throws IOException { - String groupName; - try { - groupName = master.getClusterSchema().getNamespace(desc.getTableName().getNamespaceAsString()) - .getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP); - if (groupName == null) { - groupName = RSGroupInfo.DEFAULT_GROUP; - } - } catch (MasterNotRunningException | PleaseHoldException e) { - LOG.info("Master has not initialized yet; temporarily using default RSGroup '" + - RSGroupInfo.DEFAULT_GROUP + "' for deploy of system table"); - groupName = RSGroupInfo.DEFAULT_GROUP; + @Override + public void postClearDeadServers(ObserverContext<MasterCoprocessorEnvironment> ctx, + List<ServerName> servers, List<ServerName> notClearedServers) throws IOException { + Set<Address> clearedServer = + servers.stream().filter(server -> !notClearedServers.contains(server)) + .map(ServerName::getAddress).collect(Collectors.toSet()); + if (!clearedServer.isEmpty()) { + groupAdminServer.removeServers(clearedServer); } + } - RSGroupInfo rsGroupInfo = groupAdminServer.getRSGroupInfo(groupName); - if (rsGroupInfo == null) { - throw new ConstraintException( - "Default RSGroup (" + groupName + ") for this table's " + "namespace does not exist."); + private void checkGroupExists(Optional<String> optGroupName) throws IOException { + if (optGroupName.isPresent()) { + String groupName = optGroupName.get(); + if (groupAdminServer.getRSGroupInfo(groupName) == null) { + throw new ConstraintException("Region server group " + groupName + " does not exit"); + } } + } - for (ServerName onlineServer : master.getServerManager().createDestinationServersList()) { - if (rsGroupInfo.getServers().contains(onlineServer.getAddress())) { + private boolean rsgroupHasServersOnline(TableDescriptor desc) throws IOException { + RSGroupInfo rsGroupInfo; + Optional<String> optGroupName = desc.getRegionServerGroup(); + if (optGroupName.isPresent()) { + String groupName = optGroupName.get(); + if (groupName.equals(RSGroupInfo.DEFAULT_GROUP)) { + // do not check for default group + return true; + } + rsGroupInfo = groupAdminServer.getRSGroupInfo(groupName); + if (rsGroupInfo == null) { + throw new ConstraintException( + "RSGroup " + groupName + " for table " + desc.getTableName() + " does not exist"); + } + } else { + NamespaceDescriptor nd = + master.getClusterSchema().getNamespace(desc.getTableName().getNamespaceAsString()); + String groupNameOfNs = nd.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP); + if (groupNameOfNs == null || groupNameOfNs.equals(RSGroupInfo.DEFAULT_GROUP)) { + // do not check for default group return true; } + rsGroupInfo = groupAdminServer.getRSGroupInfo(groupNameOfNs); + if (rsGroupInfo == null) { + throw new ConstraintException("RSGroup " + groupNameOfNs + " for table " + + desc.getTableName() + "(inherit from namespace) does not exist"); + } } - return false; + return master.getServerManager().createDestinationServersList().stream() + .anyMatch(onlineServer -> rsGroupInfo.containsServer(onlineServer.getAddress())); } @Override - public void preCreateTableAction(final ObserverContext<MasterCoprocessorEnvironment> ctx, - final TableDescriptor desc, final RegionInfo[] regions) throws IOException { + public void preCreateTableAction(ObserverContext<MasterCoprocessorEnvironment> ctx, + TableDescriptor desc, RegionInfo[] regions) throws IOException { + checkGroupExists(desc.getRegionServerGroup()); if (!desc.getTableName().isSystemTable() && !rsgroupHasServersOnline(desc)) { - throw new HBaseIOException("No online servers in the rsgroup, which table " + - desc.getTableName().getNameAsString() + " belongs to"); + throw new HBaseIOException("No online servers in the rsgroup for " + desc); Review comment: Just keep the old behavior. And in general, I do not think we need to check for online servers here. Region servers can die at any time and cause the rs group to have no servers in group, so users may get random behaviors and confused... ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services