Uh oh!
There was an error while loading. Please reload this page.
HBASE-22414 Interruption of moving regions in RSGroup will cause regi… - #323
Closed
sunhelly wants to merge 16 commits into
Closed
HBASE-22414 Interruption of moving regions in RSGroup will cause regi…#323sunhelly wants to merge 16 commits into
sunhelly wants to merge 16 commits into
Conversation
Apache-HBase
commented
Jun 19, 2019
🎊 +1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
| try { | ||
| this.master.getAssignmentManager().move(region); | ||
| }catch (IOException ioe){ | ||
| LOG.error("Move region {} from group failed, will retry, current retry time is {}", |
Contributor
There was a problem hiding this comment.
May get too verbose. How about log as debug on each retry, then if max number of reties has been reached, log as error?
ContributorAuthor
There was a problem hiding this comment.
Yes, I agree. But I think we need failed regions' names to move them to the target group servers after the failed call, because we can't call move tables or servers methods to move failed regions again.Maybe I can add a failed regions list in the ex message when max number of reties has been reached.What do you think about it?
ContributorAuthor
There was a problem hiding this comment.
I have changed this log level to DEBUG, and added a failed regions list in the ex message when max number of reties has been reached.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @Test | ||
| public void testFailedMoveWhenMoveServer() throws Exception { | ||
| final RSGroupInfo newGroup = addGroup(getGroupName(name.getMethodName()), 1); |
Contributor
There was a problem hiding this comment.
addGroup already invokes _ moveServerRegionsFromGroup_ indirectly, and adds an RS to it. We could simplify this test if we just create a new group from RSGroupAdminServer.addRSGroup, pick one of the RSes from assignMap, change one of its regions state, then call later call RSGroupAdminServer.moveServers?
ContributorAuthor
There was a problem hiding this comment.
In this case, I have used rsGroupAdmin.addRSGroup to add a new group without adding server. Then I create a multi-region table, choose a region to change state, and record the server that the region we changed on. In the last call RSGroupAdminServer.moveServers to move the recorded server, and check if the error message contains the region name.
| } | ||
| @Test | ||
| public void testFailedMoveWhenMoveTable() throws Exception { |
Contributor
There was a problem hiding this comment.
Same as to the tested methods, we may apply some code reuse, given how similarly the two test structures are?
ContributorAuthor
There was a problem hiding this comment.
I make a function named setARegionState to randomly choosing a region and set its state. The difference of the two UTs is at adding group and moving operation. Is this OK?
ContributorAuthor
There was a problem hiding this comment.
I will add a new skeleton function recoverRegionStateThread at the newly attached patch.
Uh oh!
There was an error while loading. Please reload this page.
sunhelly
commented
Jun 21, 2019
ContributorAuthor
@wchevreuil Thank you for reviewing, I'll change codes follow your comments. |
Apache-HBase
commented
Jun 23, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 24, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 24, 2019
🎊 +1 overall
This message was automatically generated. |
xcangCRM
requested changes
Jun 27, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| */ | ||
| private void moveServerRegionsFromGroup(Set<Address> servers, String targetGroupName) | ||
| private <T> void moveRegionsToOrFromGroup(Set<T> set, String targetGroupName, MoveType type) | ||
| throws IOException { |
Contributor
There was a problem hiding this comment.
Is this really a good idea to group "TO and FROM" into one method? It's an open question, just want to hear some reasoning from you. thanks
ContributorAuthor
There was a problem hiding this comment.
I think added To/From to method name makes codes more easy to be understood.
And just like writing in the docs, TO means move tables regions, FROM means move server regions.
| getRegionState(region).isFailedOpen()) { | ||
| continue; | ||
| for (Iterator<T> iter = newSet.iterator(); iter.hasNext(); ) { | ||
| T el = iter.next(); |
Contributor
There was a problem hiding this comment.
"el" this naming is not very good IMO.
| /** | ||
| * Move every region from servers which are currently located on these servers, | ||
| * but should not be located there. | ||
| * When move a table to a group, all regions of it must be moved to group servers (to the group). |
Contributor
There was a problem hiding this comment.
I think you should make this description clearer regarding FROM , TO, TABLE and GROUP
Apache-HBase
commented
Jun 27, 2019
🎊 +1 overall
This message was automatically generated. |
wchevreuil
requested changes
Jul 1, 2019
wchevreuil
left a comment
Contributor
There was a problem hiding this comment.
I agree with @xcangCRM , my opinion is that this single method resulting from a merge of previously two moveServerRegionsFromGroup and moveTableRegionsToGroup does not look easier to understand.
My earlier suggestion was just to define a common skeleton method that would be reused by both moveServerRegionsFromGroup and moveTableRegionsToGroup, given that the loop structures were basically the same, with variation only on Set types and validation logic. I believe this could be achieved by defining a common method using lambdas and generics, for example, something like:
...private <T> void moveRegionsBetweenGroups(Set<T> regionsOwners, String targetGroupName, Function<T, List<RegionInfo>> getRegionsInfo, Function<RegionInfo,Boolean> validation, Function<T,String> getOwnerName) throws IOException
...
Then _ moveServerRegionsFromGroup_ and _ moveTableRegionsToGroup_ would reuse it, defining specific validation logic, for example moveServerRegionsFromGroup would something such as below:
moveRegionsBetweenGroups(servers, targetGroupName, rs -> getRegions(rs), info -> { return getRSGroupInfo(targetGroupName).containsTable(info.getTable());}, rs -> rs.getHostname());
wchevreuil
commented
Jul 1, 2019
Contributor
Pushed a commit with an example for the skeleton mentioned previously. Let me know if you guys agree with this. |
Apache-HBase
commented
Jul 1, 2019
💔 -1 overall
This message was automatically generated. |
sunhelly
commented
Jul 2, 2019
ContributorAuthor
@wchevreuil I agree with you. |
wchevreuil
commented
Jul 2, 2019
Contributor
Apache-HBase
commented
Jul 2, 2019
🎊 +1 overall
This message was automatically generated. |
sunhelly
commented
Jul 3, 2019
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Jul 8, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 8, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 8, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 9, 2019
🎊 +1 overall
This message was automatically generated. |
sunhelly
commented
Jul 9, 2019
ContributorAuthor
@wchevreuil I have updated the patch, please help to review. |
wchevreuil
commented
Jul 9, 2019
Contributor
Thanks for the updates, @sunhelly ! LGTM for the latest commits, had already resolved the conversation about exception handler when retries exhausted. Can you share your thoughts on the remaining comments? Let me know if those make sense. |
message to contains region name.
Apache-HBase
commented
Jul 10, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 10, 2019
🎊 +1 overall
This message was automatically generated. |
sunhelly
commented
Jul 10, 2019
ContributorAuthor
@wchevreuil I have replied on the remaining comments, and updated my patch again. |
wchevreuil
left a comment
Contributor
There was a problem hiding this comment.
Thanks @sunhelly, latest changes look good. Had made some additional review comments, please share your thoughts on those once you have a chance.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
sunhelly
commented
Jul 12, 2019
ContributorAuthor
@wchevreuil I have updated the codes as replied in all the comments. Please review again. |
Apache-HBase
commented
Jul 12, 2019
🎊 +1 overall
This message was automatically generated. |
| Pair<ServerName, RegionStateNode> gotPair = createTableAndSetARegionState(newGroup, 5); | ||
| try{ | ||
| rsGroupAdmin.moveTables(Sets.newHashSet(tableName), newGroup.getName()); | ||
| fail("move tables to group should fail"); |
Contributor
There was a problem hiding this comment.
Small nit: let's mention the test expected a IOE that didn't happen.
ContributorAuthor
There was a problem hiding this comment.
Thanks, I'll change the fail message to mention the expected exception.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wchevreuil
commented
Jul 14, 2019
Contributor
Hi @sunhelly ! Latest changes looking great! Had just 3 more comments, all small nits. Let me know if you think those make sense, then I guess we'll be ready to push this PR. |
sunhelly
commented
Jul 15, 2019
ContributorAuthor
@wchevreuil Thanks for your comments, I have updated the patch. |
wchevreuil
commented
Jul 15, 2019
Contributor
Squashed commits locally, then pushed to master. Closing this PR now, thanks for contributing @sunhelly ! |
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
4 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.
…ons on wrong rs