-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-24112 [RSGroup] Support renaming rsgroup #1435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
9c473f0
6105914
d4163d6
65a7b19
1f2ade5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,9 @@ public class RSGroupInfo { | |
| // Keep servers in a sorted set so has an expected ordering when displayed. | ||
| private final SortedSet<Address> 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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| * <h2>Concurrency</h2> 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. | ||
| * <p/> | ||
| * 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 | ||
|
|
@@ -1221,4 +1221,32 @@ 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<String, RSGroupInfo> rsGroupMap = holder.groupName2Group; | ||
| Map<String, RSGroupInfo> newGroupMap = Maps.newHashMap(rsGroupMap); | ||
| newGroupMap.remove(oldRSG.getName()); | ||
| RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); | ||
| newGroupMap.put(newName, newRSG); | ||
| flushConfig(newGroupMap); | ||
| Set<TableName> updateTables = new HashSet<>(); | ||
|
||
| TableDescriptors tableDescriptors = masterServices.getTableDescriptors(); | ||
| for (Map.Entry<String, TableDescriptor> table : tableDescriptors.getAll().entrySet()) { | ||
| Optional<String> rsgroup = table.getValue().getRegionServerGroup(); | ||
| if (!rsgroup.isPresent()) { | ||
| continue; | ||
| } | ||
| if (rsgroup.get().equals(oldName)) { | ||
| updateTables.add(table.getValue().getTableName()); | ||
| } | ||
| } | ||
| setRSGroup(updateTables, newName); | ||
Apache9 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -468,4 +468,33 @@ 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()); | ||
| TEST_UTIL.waitFor(1000, | ||
| (Waiter.Predicate<Exception>) () -> | ||
| ADMIN.getRSGroup("oldgroup").getServers().size() == 2); | ||
| 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<Address> 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this redundant assert?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The for loop above is to check the servers are exactly the same before renaming. And the match is the number of 「exactly the same」. Not redundant regarding to its purpose.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I agree this is useful assertion. Actually I was wondering if we can add more context to this test as well as assertions. For example, we create and assign another table to another rsgroup. So the "if" clause checking rsgroup name in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Get your idea. Fixed in new commit.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes the test now is very comprehensive. Thanks, |
||
| assertEquals(newgroup.getName(), ADMIN.getRSGroup(tb).getName()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to provide any validation for both strings? e.g. non-empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is ok to leave it as it is, server side will throw ConstraintException if both are empty. Other methods like addRSGroup neither checks non-empty.