Uh oh!
There was an error while loading. Please reload this page.
HBASE-24112 [RSGroup] Support renaming rsgroup - #1435
Merged
Conversation
Apache-HBase
commented
Apr 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 4, 2020
🎊 +1 overall
This message was automatically generated. |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Should we provide shell command for rename? Or maybe pass some attributes to existing rsgroup create command to indicate it is rename call? Or as of now Admin API is enough?
| } | ||
| @Override | ||
| public CompletableFuture<Void> renameRSGroup(String oldName, String newName) { |
Contributor
There was a problem hiding this comment.
Would you like to provide any validation for both strings? e.g. non-empty
ContributorAuthor
There was a problem hiding this comment.
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.
| Map<String, RSGroupInfo> newGroupMap = Maps.newHashMap(rsGroupMap); | ||
| newGroupMap.remove(oldRSG.getName()); | ||
| RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); | ||
| newGroupMap.put(newRSG.getName(), newRSG); |
Contributor
There was a problem hiding this comment.
nit: simplify to newGroupMap.put(newName, newRSG); ?
| TableName tb = TableName.valueOf("testRename"); | ||
| TEST_UTIL.createTable(tb, "tr"); | ||
| ADMIN.setRSGroup(Sets.newHashSet(tb), oldgroup.getName()); | ||
| Thread.sleep(500); |
Contributor
There was a problem hiding this comment.
Instead of sleep(), we can use HBASE_TESTING_UTILITY.waitFor:
e.g.
HBASE_TESTING_UTILITY.waitFor(1000,
() -> {
oldgroup = ADMIN.getRSGroup(oldgroup.getName());
return oldgroup.getServers().size() == 2 && ADMIN.getRSGroup(tb).getName().equals(oldgroup.getName());
});
This way we will ensure, we wait for specific period of time until our predicate returns true.
| match++; | ||
| } | ||
| } | ||
| assertEquals(servers.size(), match); |
Contributor
There was a problem hiding this comment.
Is this redundant assert? assertEquals(servers.size(), newgroup.getServers().size()); is already taking care of it?
ContributorAuthor
There was a problem hiding this comment.
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.
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 RSGroupInfoManagerImpl.java will get tested. Otherwise this test may still pass even when we remove the "if". By the "if" I mean:
if (rsgroup.get().equals(oldName)) {
updateTables.add(table.getValue().getTableName());
}
ContributorAuthor
There was a problem hiding this comment.
Get your idea. Fixed in new commit.
Member
There was a problem hiding this comment.
Yes the test now is very comprehensive. Thanks,
Reidddddd
commented
Apr 6, 2020
ContributorAuthor
Plan to do the shell command in a separate JIRA, to make backport easier. |
Apache9
requested changes
Apr 6, 2020
| private final SortedSet<Address> servers; | ||
| // Keep tables sorted too. | ||
| // TODO: Don't understand why all these should be deprecated. we have table -> rsgroup mapping. |
Contributor
There was a problem hiding this comment.
Because we do not store the table informations in RSGroupInfo anymore, if you use the new API to get RSGroupInfo, the tables will always be empty.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Apr 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 6, 2020
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Apr 7, 2020
Contributor
I do not think a green UT result can reduce the concern on the 'unexepcted behavior'. It does not happen now does mean it will not happen in the future. The root problem here, is that, after the renaming, the TableDescriptor at RS side still reference the old name, if we write some code at RS side that retrieve this value and do something, we may be in trouble. |
Reidddddd
commented
Apr 7, 2020
ContributorAuthor
Get it. It means only reopening the regions can get the most updated TD on RS side in this case. What's your suggestion to this concern? |
Apache9
commented
Apr 7, 2020
Contributor
I do not have an idea yet. Maybe we could find a way to update the table descriptor for a region at RS side without reopening it? |
ContributorAuthor
Just rolled back to the previous version, the one using setRSGroups. It is the appropriate & right approach by now. |
Apache-HBase
commented
Apr 7, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 7, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 7, 2020
💔 -1 overall
This message was automatically generated. |
liuml07
reviewed
Apr 8, 2020
liuml07
left a comment
Member
There was a problem hiding this comment.
I do not have enough context to be +1. Posting some random comments here
| RSGroupInfo newRSG = new RSGroupInfo(newName, oldRSG.getServers()); | ||
| newGroupMap.put(newName, newRSG); | ||
| flushConfig(newGroupMap); | ||
| Set<TableName> updateTables = new HashSet<>(); |
Member
There was a problem hiding this comment.
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());
}
}
can be replaced with, if stream is preferred, following code (not tested):
Set<TableName> updateTables = masterServices.getTableDescriptors().getAll().values()
.stream()
.filter(t -> oldName.equals(t.getRegionServerGroup().orElse(null)))
.map(TableDescriptor::getTableName)
.collect(Collectors.toSet());
| match++; | ||
| } | ||
| } | ||
| assertEquals(servers.size(), match); |
Member
There was a problem hiding this comment.
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 RSGroupInfoManagerImpl.java will get tested. Otherwise this test may still pass even when we remove the "if". By the "if" I mean:
if (rsgroup.get().equals(oldName)) {
updateTables.add(table.getValue().getTableName());
}
…nsure it works as expectation
Apache-HBase
commented
Apr 8, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 8, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 8, 2020
💔 -1 overall
This message was automatically generated. |
Reidddddd
commented
Apr 9, 2020
ContributorAuthor
Shall we proceed? And regarding to this idea:
I think it worth an independent JIRA, quite big might be. |
Apache9
approved these changes
Apr 13, 2020
Apache9
left a comment
Contributor
There was a problem hiding this comment.
Still a little worry about adding a new method which is not perfect, but anyway, it could make it easier for user to rename a rs group. Maybe we could find a way to improve the implementation in the future.
+1.
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Duo Zhang <zhangduo@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Duo Zhang <zhangduo@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Duo Zhang <zhangduo@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Duo Zhang <zhangduo@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Duo Zhang <zhangduo@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA link: https://issues.apache.org/jira/browse/HBASE-24112