From 9c473f0fe1710165950409b90f7de2ef41bb5768 Mon Sep 17 00:00:00 2001 From: Reid Chan Date: Sat, 4 Apr 2020 23:48:22 +0800 Subject: [PATCH 1/5] HBASE-24112 [RSGroup] Support renaming rsgroup --- .../org/apache/hadoop/hbase/client/Admin.java | 9 ++++++ .../hbase/client/AdminOverAsyncAdmin.java | 5 ++++ .../hadoop/hbase/client/AsyncAdmin.java | 8 +++++ .../hadoop/hbase/client/AsyncHBaseAdmin.java | 5 ++++ .../hbase/client/RawAsyncHBaseAdmin.java | 19 ++++++++++++ .../hadoop/hbase/rsgroup/RSGroupInfo.java | 3 ++ .../main/protobuf/server/master/Master.proto | 3 ++ .../server/rsgroup/RSGroupAdmin.proto | 11 +++++++ .../hbase/coprocessor/MasterObserver.java | 18 ++++++++++++ .../hbase/master/MasterCoprocessorHost.java | 20 +++++++++++++ .../hbase/master/MasterRpcServices.java | 26 ++++++++++++++++- .../rsgroup/DisabledRSGroupInfoManager.java | 4 +++ .../rsgroup/RSGroupAdminServiceImpl.java | 27 ++++++++++++++++- .../hbase/rsgroup/RSGroupInfoManager.java | 8 +++++ .../hbase/rsgroup/RSGroupInfoManagerImpl.java | 29 +++++++++++++++++++ .../hbase/rsgroup/TestRSGroupsAdmin1.java | 27 +++++++++++++++++ .../hbase/rsgroup/VerifyingRSGroupAdmin.java | 6 ++++ .../hbase/thrift2/client/ThriftAdmin.java | 4 +++ 18 files changed, 230 insertions(+), 2 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java index 813e8108e425..bd4e3051bcde 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java @@ -2396,4 +2396,13 @@ Pair, List> getConfiguredNamespacesAndTablesInRSGroup(St * @throws IOException if a remote or network exception occurs */ boolean balanceRSGroup(String groupName) throws IOException; + + /** + * Rename rsgroup + * @param oldName old rsgroup name + * @param newName new rsgroup name + * @throws IOException if a remote or network exception occurs + */ + void renameRSGroup(String oldName, String newName) throws IOException; + } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java index e3c888c10a57..8f2d2888383d 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java @@ -1036,4 +1036,9 @@ public RSGroupInfo getRSGroup(TableName tableName) throws IOException { public void setRSGroup(Set tables, String groupName) throws IOException { get(admin.setRSGroup(tables, groupName)); } + + @Override + public void renameRSGroup(String oldName, String newName) throws IOException { + get(admin.renameRSGroup(oldName, newName)); + } } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java index 02d9f3b0019c..c6ef55c97a2c 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java @@ -1631,4 +1631,12 @@ CompletableFuture> getSlowLogResponses(final Set balanceRSGroup(String groupName); + + /** + * Rename rsgroup + * @param oldName old rsgroup name + * @param newName new rsgroup name + * @throws IOException if a remote or network exception occurs + */ + CompletableFuture renameRSGroup(String oldName, String newName); } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java index c3f0da128a97..8b29ac97c4d5 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java @@ -913,4 +913,9 @@ public CompletableFuture getRSGroup(TableName tableName) { public CompletableFuture setRSGroup(Set tables, String groupName) { return wrap(rawAdmin.setRSGroup(tables, groupName)); } + + @Override + public CompletableFuture renameRSGroup(String oldName, String newName) { + return wrap(rawAdmin.renameRSGroup(oldName, newName)); + } } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java index 6b504ee23cc5..a9110380753c 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java @@ -309,6 +309,8 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveRSGroupResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersResponse; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos.AddReplicationPeerRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos.AddReplicationPeerResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos.DisableReplicationPeerRequest; @@ -4146,4 +4148,21 @@ public CompletableFuture getRSGroup(String groupName) { resp -> resp.hasRSGroupInfo() ? ProtobufUtil.toGroupInfo(resp.getRSGroupInfo()) : null))) .call(); } + + @Override + public CompletableFuture renameRSGroup(String oldName, String newName) { + return this. newMasterCaller() + .action( + ( + (controller, stub) -> this. call( + controller, + stub, + RenameRSGroupRequest.newBuilder().setOldRsgroupName(oldName).setNewRsgroupName(newName) + .build(), + (s, c, req, done) -> s.renameRSGroup(c, req, done), + resp -> null + ) + ) + ).call(); + } } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java index 817e23744e36..c2b0d26e247d 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java @@ -38,6 +38,9 @@ public class RSGroupInfo { // Keep servers in a sorted set so has an expected ordering when displayed. private final SortedSet
servers; // Keep tables sorted too. + + // TODO: Don't understand why all these should be deprecated. we have table -> rsgroup mapping. + // Isn't it reasonable to have rsgroup -> tables mapping as well. Any contradictory? /** * @deprecated Since 3.0.0, will be removed in 4.0.0. The rsgroup information will be stored in * the configuration of a table so this will be removed. diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto index 665907a964d5..aae67fc7224f 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto @@ -1119,6 +1119,9 @@ service MasterService { rpc GetConfiguredNamespacesAndTablesInRSGroup(GetConfiguredNamespacesAndTablesInRSGroupRequest) returns (GetConfiguredNamespacesAndTablesInRSGroupResponse); + + rpc RenameRSGroup(RenameRSGroupRequest) + returns (RenameRSGroupResponse); } // HBCK Service definitions. diff --git a/hbase-protocol-shaded/src/main/protobuf/server/rsgroup/RSGroupAdmin.proto b/hbase-protocol-shaded/src/main/protobuf/server/rsgroup/RSGroupAdmin.proto index e279e31d95e0..860c5aa9c5cb 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/rsgroup/RSGroupAdmin.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/rsgroup/RSGroupAdmin.proto @@ -139,6 +139,14 @@ message GetConfiguredNamespacesAndTablesInRSGroupResponse { repeated TableName table_name = 2; } +message RenameRSGroupRequest { + required string old_rsgroup_name = 1; + required string new_rsgroup_name = 2; +} + +message RenameRSGroupResponse { +} + service RSGroupAdminService { rpc GetRSGroupInfo(GetRSGroupInfoRequest) returns (GetRSGroupInfoResponse); @@ -172,4 +180,7 @@ service RSGroupAdminService { rpc RemoveServers(RemoveServersRequest) returns (RemoveServersResponse); + + rpc RenameRSGroup(RenameRSGroupRequest) + returns (RenameRSGroupResponse); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java index 900a99c82692..f7487ea225cf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java @@ -1340,6 +1340,24 @@ default void preListTablesInRSGroup(final ObserverContext ctx, final String groupName) throws IOException {} + /** + * Called before rename rsgroup. + * @param ctx the environment to interact with the framework and master + * @param oldName old rsgroup name + * @param newName new rsgroup name + */ + default void preRenameRSGroup(final ObserverContext ctx, + final String oldName, final String newName) throws IOException {} + + /** + * Called after rename rsgroup. + * @param ctx the environment to interact with the framework and master + * @param oldName old rsgroup name + * @param newName new rsgroup name + */ + default void postRenameRSGroup(final ObserverContext ctx, + final String oldName, final String newName) throws IOException {} + /** * Called before getting the configured namespaces and tables in the region server group. * @param ctx the environment to interact with the framework and master diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java index 558c82c1e827..4d0f25c0b731 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java @@ -1542,6 +1542,26 @@ protected void call(MasterObserver observer) throws IOException { }); } + public void preRenameRSGroup(final String oldName, final String newName) throws IOException { + execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { + + @Override + protected void call(MasterObserver observer) throws IOException { + observer.preRenameRSGroup(this, oldName, newName); + } + }); + } + + public void postRenameRSGroup(final String oldName, final String newName) throws IOException { + execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { + + @Override + protected void call(MasterObserver observer) throws IOException { + observer.postRenameRSGroup(this, oldName, newName); + } + }); + } + public void preGetConfiguredNamespacesAndTablesInRSGroup(final String groupName) throws IOException { execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index a12ac7d4e700..56349fa3ae3b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -341,6 +341,8 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveRSGroupResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersResponse; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.FileArchiveNotificationRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.FileArchiveNotificationResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.GetLastFlushedSequenceIdRequest; @@ -2456,7 +2458,7 @@ public FileArchiveNotificationResponse reportFileArchival(RpcController controll throw new ServiceException(e); } } - + // HBCK Services @Override @@ -3218,4 +3220,26 @@ public ListTablesInRSGroupResponse listTablesInRSGroup(RpcController controller, } return builder.build(); } + + @Override + public RenameRSGroupResponse renameRSGroup(RpcController controller, + RenameRSGroupRequest request) throws ServiceException { + RenameRSGroupResponse.Builder builder = RenameRSGroupResponse.newBuilder(); + String oldRSGroup = request.getOldRsgroupName(); + String newRSGroup = request.getNewRsgroupName(); + LOG.info("{} rename rsgroup from {} to {} ", + master.getClientIdAuditPrefix(), oldRSGroup, newRSGroup); + try { + if (master.getMasterCoprocessorHost() != null) { + master.getMasterCoprocessorHost().preRenameRSGroup(oldRSGroup, newRSGroup); + } + master.getRSGroupInfoManager().renameRSGroup(oldRSGroup, newRSGroup); + if (master.getMasterCoprocessorHost() != null) { + master.getMasterCoprocessorHost().postRenameRSGroup(oldRSGroup, newRSGroup); + } + } catch (IOException e) { + throw new ServiceException(e); + } + return builder.build(); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/DisabledRSGroupInfoManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/DisabledRSGroupInfoManager.java index c76da70f259d..b86b471624d9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/DisabledRSGroupInfoManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/DisabledRSGroupInfoManager.java @@ -121,4 +121,8 @@ public String determineRSGroupInfoForTable(TableName tableName) { return RSGroupInfo.DEFAULT_GROUP; } + @Override + public void renameRSGroup(String oldName, String newName) throws IOException { + } + } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServiceImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServiceImpl.java index fa816e64c3b7..241fd6522e7c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServiceImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServiceImpl.java @@ -67,6 +67,8 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveRSGroupResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RemoveServersResponse; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RSGroupAdminProtos.RenameRSGroupResponse; /** * Implementation of RSGroupAdminService defined in RSGroupAdmin.proto. This class calls @@ -396,4 +398,27 @@ public void removeServers(RpcController controller, RemoveServersRequest request done.run(builder.build()); } -} \ No newline at end of file + @Override + public void renameRSGroup(RpcController controller, RenameRSGroupRequest request, + RpcCallback done) { + String oldRSGroup = request.getOldRsgroupName(); + String newRSGroup = request.getNewRsgroupName(); + LOG.info("{} rename rsgroup from {} to {}", + master.getClientIdAuditPrefix(), oldRSGroup, newRSGroup); + + RenameRSGroupResponse.Builder builder = RenameRSGroupResponse.newBuilder(); + try { + if (master.getMasterCoprocessorHost() != null) { + master.getMasterCoprocessorHost().preRenameRSGroup(oldRSGroup, newRSGroup); + } + rsGroupInfoManager.renameRSGroup(oldRSGroup, newRSGroup); + if (master.getMasterCoprocessorHost() != null) { + master.getMasterCoprocessorHost().postRenameRSGroup(oldRSGroup, newRSGroup); + } + } catch (IOException e) { + CoprocessorRpcUtils.setControllerException(controller, e); + } + done.run(builder.build()); + } + +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManager.java index a35c4c0a2baa..10008f9942ad 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManager.java @@ -106,4 +106,12 @@ static RSGroupInfoManager create(MasterServices master) throws IOException { */ String determineRSGroupInfoForTable(TableName tableName); + /** + * Rename rsgroup + * @param oldName old rsgroup name + * @param newName new rsgroup name + * @throws IOException + */ + void renameRSGroup(String oldName, String newName) throws IOException; + } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java index 5b2e8bf4c9d7..0cc2e88f2444 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java @@ -1221,4 +1221,33 @@ public String determineRSGroupInfoForTable(TableName tableName) { return script.getRSGroup(tableName.getNamespaceAsString(), tableName.getQualifierAsString()); } + @Override + public synchronized void renameRSGroup(String oldName, String newName) throws IOException { + if (oldName.equals(RSGroupInfo.DEFAULT_GROUP)) { + throw new ConstraintException(RSGroupInfo.DEFAULT_GROUP + " can't be rename"); + } + checkGroupName(newName); + + RSGroupInfo oldRSG = getRSGroupInfo(oldName); + Map rsGroupMap = holder.groupName2Group; + Map newGroupMap = Maps.newHashMap(rsGroupMap); + newGroupMap.remove(oldRSG.getName()); + RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); + newGroupMap.put(newRSG.getName(), newRSG); + flushConfig(newGroupMap); + + TableDescriptors tableDescriptors = masterServices.getTableDescriptors(); + Set updateTables = new HashSet<>(); + for (Map.Entry table : tableDescriptors.getAll().entrySet()) { + Optional rsgroup = table.getValue().getRegionServerGroup(); + if (!rsgroup.isPresent()) { + continue; + } + if (rsgroup.get().equals(oldName)) { + updateTables.add(table.getValue().getTableName()); + } + } + setRSGroup(updateTables, newName); + } + } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java index 33077ccfd44a..3c3b8b1991c4 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java @@ -468,4 +468,31 @@ public boolean evaluate() throws Exception { // Cleanup TEST_UTIL.deleteTable(tn1); } + + @Test + public void testRenameRSGroup() throws Exception { + // Add rsgroup, and assign 2 servers and a table to it. + RSGroupInfo oldgroup = addGroup("oldgroup", 2); + TableName tb = TableName.valueOf("testRename"); + TEST_UTIL.createTable(tb, "tr"); + ADMIN.setRSGroup(Sets.newHashSet(tb), oldgroup.getName()); + Thread.sleep(500); + oldgroup = ADMIN.getRSGroup(oldgroup.getName()); + assertEquals(2, oldgroup.getServers().size()); + assertEquals(oldgroup.getName(), ADMIN.getRSGroup(tb).getName()); + + // Rename rsgroup + ADMIN.renameRSGroup(oldgroup.getName(), "newgroup"); + Set
servers = oldgroup.getServers(); + RSGroupInfo newgroup = ADMIN.getRSGroup("newgroup"); + assertEquals(servers.size(), newgroup.getServers().size()); + int match = 0; + for (Address addr : newgroup.getServers()) { + if (servers.contains(addr)) { + match++; + } + } + assertEquals(servers.size(), match); + assertEquals(newgroup.getName(), ADMIN.getRSGroup(tb).getName()); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java index 8ff50f1e5fb4..7e1ec6e1dd21 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java @@ -816,6 +816,12 @@ public boolean balanceRSGroup(String groupName) throws IOException { return admin.balanceRSGroup(groupName); } + @Override + public void renameRSGroup(String oldName, String newName) throws IOException { + admin.renameRSGroup(oldName, newName); + verify(); + } + private void verify() throws IOException { Map groupMap = Maps.newHashMap(); Set zList = Sets.newHashSet(); diff --git a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java index 677adc9f2fb8..2e248977183b 100644 --- a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java +++ b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java @@ -1257,4 +1257,8 @@ public List listTablesInRSGroup(String groupName) throws IOException throw new NotImplementedException("setRSGroup not supported in ThriftAdmin"); } + @Override + public void renameRSGroup(String oldName, String newName) throws IOException { + throw new NotImplementedException("renameRSGroup not supported in ThriftAdmin"); + } } From 610591418d65edf5fa7bc9d4c00cb82a0dfca606 Mon Sep 17 00:00:00 2001 From: Reid Chan Date: Mon, 6 Apr 2020 11:22:26 +0800 Subject: [PATCH 2/5] Address review comments --- .../apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java | 2 +- .../org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java index 0cc2e88f2444..6e10a17b46ef 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java @@ -1233,7 +1233,7 @@ public synchronized void renameRSGroup(String oldName, String newName) throws IO Map newGroupMap = Maps.newHashMap(rsGroupMap); newGroupMap.remove(oldRSG.getName()); RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); - newGroupMap.put(newRSG.getName(), newRSG); + newGroupMap.put(newName, newRSG); flushConfig(newGroupMap); TableDescriptors tableDescriptors = masterServices.getTableDescriptors(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java index 3c3b8b1991c4..9002412fd96d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java @@ -476,7 +476,9 @@ public void testRenameRSGroup() throws Exception { TableName tb = TableName.valueOf("testRename"); TEST_UTIL.createTable(tb, "tr"); ADMIN.setRSGroup(Sets.newHashSet(tb), oldgroup.getName()); - Thread.sleep(500); + TEST_UTIL.waitFor(1000, + (Waiter.Predicate) () -> + ADMIN.getRSGroup("oldgroup").getServers().size() == 2); oldgroup = ADMIN.getRSGroup(oldgroup.getName()); assertEquals(2, oldgroup.getServers().size()); assertEquals(oldgroup.getName(), ADMIN.getRSGroup(tb).getName()); From d4163d68ccfa1f84ee05a6ba5b1b2fc44769ce8a Mon Sep 17 00:00:00 2001 From: Reid Chan Date: Mon, 6 Apr 2020 12:23:06 +0800 Subject: [PATCH 3/5] Avoid reopen regions, since there's no location changes --- .../hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java index 6e10a17b46ef..6ae025fd3f67 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java @@ -104,8 +104,8 @@ * persistence store for the group information. It also makes use of zookeeper to store group * information needed for bootstrapping during offline mode. *

Concurrency

RSGroup state is kept locally in Maps. There is a rsgroup name to cached - * RSGroupInfo Map at {@link #rsGroupMap}. These Maps are persisted to the hbase:rsgroup table - * (and cached in zk) on each modification. + * RSGroupInfo Map at {@link RSGroupInfoHolder#groupName2Group}. + * These Maps are persisted to the hbase:rsgroup table (and cached in zk) on each modification. *

* Mutations on state are synchronized but reads can continue without having to wait on an instance * monitor, mutations do wholesale replace of the Maps on update -- Copy-On-Write; the local Maps of @@ -1237,17 +1237,17 @@ public synchronized void renameRSGroup(String oldName, String newName) throws IO flushConfig(newGroupMap); TableDescriptors tableDescriptors = masterServices.getTableDescriptors(); - Set updateTables = new HashSet<>(); for (Map.Entry table : tableDescriptors.getAll().entrySet()) { Optional rsgroup = table.getValue().getRegionServerGroup(); if (!rsgroup.isPresent()) { continue; } if (rsgroup.get().equals(oldName)) { - updateTables.add(table.getValue().getTableName()); + masterServices.getTableDescriptors().update( + TableDescriptorBuilder.newBuilder(table.getValue()).setRegionServerGroup(newName).build() + ); } } - setRSGroup(updateTables, newName); } } From 65a7b19f9b464b67298e7b926c505174c7d8f05c Mon Sep 17 00:00:00 2001 From: Reid Chan Date: Tue, 7 Apr 2020 18:29:02 +0800 Subject: [PATCH 4/5] Rolled back to previous version which is the right approach so far --- .../hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java index 6ae025fd3f67..40a76e0672f6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java @@ -1235,7 +1235,7 @@ public synchronized void renameRSGroup(String oldName, String newName) throws IO RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); newGroupMap.put(newName, newRSG); flushConfig(newGroupMap); - + Set updateTables = new HashSet<>(); TableDescriptors tableDescriptors = masterServices.getTableDescriptors(); for (Map.Entry table : tableDescriptors.getAll().entrySet()) { Optional rsgroup = table.getValue().getRegionServerGroup(); @@ -1243,11 +1243,10 @@ public synchronized void renameRSGroup(String oldName, String newName) throws IO continue; } if (rsgroup.get().equals(oldName)) { - masterServices.getTableDescriptors().update( - TableDescriptorBuilder.newBuilder(table.getValue()).setRegionServerGroup(newName).build() - ); + updateTables.add(table.getValue().getTableName()); } } + setRSGroup(updateTables, newName); } } From 1f2ade5bd2af1a139bf3b979f6ae0d8b08b27d81 Mon Sep 17 00:00:00 2001 From: Reid Chan Date: Wed, 8 Apr 2020 11:40:47 +0800 Subject: [PATCH 5/5] Use lambda expression and add more context in TestRSGroupsAdmin1 to ensure it works as expectation --- .../hadoop/hbase/rsgroup/RSGroupInfo.java | 2 -- .../hbase/rsgroup/RSGroupInfoManagerImpl.java | 17 +++++------- .../hbase/rsgroup/TestRSGroupsAdmin1.java | 27 ++++++++++++++----- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java index c2b0d26e247d..5d70d1b30220 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java @@ -39,8 +39,6 @@ public class RSGroupInfo { private final SortedSet

servers; // Keep tables sorted too. - // TODO: Don't understand why all these should be deprecated. we have table -> rsgroup mapping. - // Isn't it reasonable to have rsgroup -> tables mapping as well. Any contradictory? /** * @deprecated Since 3.0.0, will be removed in 4.0.0. The rsgroup information will be stored in * the configuration of a table so this will be removed. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java index 40a76e0672f6..9e8c635cb57d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java @@ -1235,17 +1235,12 @@ public synchronized void renameRSGroup(String oldName, String newName) throws IO RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); newGroupMap.put(newName, newRSG); flushConfig(newGroupMap); - Set updateTables = new HashSet<>(); - TableDescriptors tableDescriptors = masterServices.getTableDescriptors(); - for (Map.Entry table : tableDescriptors.getAll().entrySet()) { - Optional rsgroup = table.getValue().getRegionServerGroup(); - if (!rsgroup.isPresent()) { - continue; - } - if (rsgroup.get().equals(oldName)) { - updateTables.add(table.getValue().getTableName()); - } - } + Set updateTables = + masterServices.getTableDescriptors().getAll().values() + .stream() + .filter(t -> oldName.equals(t.getRegionServerGroup().orElse(null))) + .map(TableDescriptor::getTableName) + .collect(Collectors.toSet()); setRSGroup(updateTables, newName); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java index 9002412fd96d..1879d29302b4 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java @@ -473,15 +473,29 @@ public boolean evaluate() throws Exception { public void testRenameRSGroup() throws Exception { // Add rsgroup, and assign 2 servers and a table to it. RSGroupInfo oldgroup = addGroup("oldgroup", 2); - TableName tb = TableName.valueOf("testRename"); - TEST_UTIL.createTable(tb, "tr"); - ADMIN.setRSGroup(Sets.newHashSet(tb), oldgroup.getName()); + TableName tb1 = TableName.valueOf("testRename"); + TEST_UTIL.createTable(tb1, "tr"); + ADMIN.setRSGroup(Sets.newHashSet(tb1), oldgroup.getName()); TEST_UTIL.waitFor(1000, (Waiter.Predicate) () -> - ADMIN.getRSGroup("oldgroup").getServers().size() == 2); + ADMIN.getRSGroup(tb1).getServers().size() == 2); oldgroup = ADMIN.getRSGroup(oldgroup.getName()); assertEquals(2, oldgroup.getServers().size()); - assertEquals(oldgroup.getName(), ADMIN.getRSGroup(tb).getName()); + assertEquals(oldgroup.getName(), ADMIN.getRSGroup(tb1).getName()); + + // Another rsgroup and table for verification + // that they are unchanged during we're renaming oldgroup. + RSGroupInfo normal = addGroup("normal", 1); + TableName tb2 = TableName.valueOf("unmovedTable"); + TEST_UTIL.createTable(tb2, "ut"); + ADMIN.setRSGroup(Sets.newHashSet(tb2), normal.getName()); + TEST_UTIL.waitFor(1000, + (Waiter.Predicate) () -> + ADMIN.getRSGroup(tb2).getServers().size() == 1); + normal = ADMIN.getRSGroup(normal.getName()); + assertEquals(1, normal.getServers().size()); + assertEquals(normal.getName(), ADMIN.getRSGroup(tb2).getName()); + // Rename rsgroup ADMIN.renameRSGroup(oldgroup.getName(), "newgroup"); @@ -495,6 +509,7 @@ public void testRenameRSGroup() throws Exception { } } assertEquals(servers.size(), match); - assertEquals(newgroup.getName(), ADMIN.getRSGroup(tb).getName()); + assertEquals(newgroup.getName(), ADMIN.getRSGroup(tb1).getName()); + assertEquals(normal.getName(), ADMIN.getRSGroup(tb2).getName()); } }