HBASE-22414 Interruption of moving regions in RSGroup will cause regi… #323
Conversation
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
…ons on wrong rs
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Jun 20, 2019
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Outdated
Show resolved
Hide resolved
wchevreuil
reviewed
Jun 20, 2019
| try { | ||
| this.master.getAssignmentManager().move(region); | ||
| }catch (IOException ioe){ | ||
| LOG.error("Move region {} from group failed, will retry, current retry time is {}", |
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?
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?
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.
wchevreuil
reviewed
Jun 20, 2019
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Outdated
Show resolved
Hide resolved
wchevreuil
reviewed
Jun 20, 2019
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Show resolved
Hide resolved
wchevreuil
reviewed
Jun 20, 2019
| @@ -459,4 +463,197 @@ public boolean evaluate() throws Exception { | |||
| Assert.assertEquals(null, rsGroupAdmin.getRSGroupInfo(fooGroup.getName())); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testFailedMoveWhenMoveServer() throws Exception { | |||
| final RSGroupInfo newGroup = addGroup(getGroupName(name.getMethodName()), 1); | |||
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?
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.
wchevreuil
reviewed
Jun 20, 2019
| } | ||
|
|
||
| @Test | ||
| public void testFailedMoveWhenMoveTable() throws Exception { |
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?
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?
There was a problem hiding this comment.
I will add a new skeleton function recoverRegionStateThread at the newly attached patch.
wchevreuil
reviewed
Jun 20, 2019
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Show resolved
Hide resolved
|
@wchevreuil Thank you for reviewing, I'll change codes follow your comments. |
…ons on wrong rs
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
xcangCRM
requested changes
Jun 27, 2019
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Show resolved
Hide resolved
| */ | ||
| private void moveServerRegionsFromGroup(Set<Address> servers, String targetGroupName) | ||
| private <T> void moveRegionsToOrFromGroup(Set<T> set, String targetGroupName, MoveType type) | ||
| throws IOException { |
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
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(); |
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). |
There was a problem hiding this comment.
I think you should make this description clearer regarding FROM , TO, TABLE and GROUP
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
requested changes
Jul 1, 2019
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());
|
Pushed a commit with an example for the skeleton mentioned previously. Let me know if you guys agree with this. |
|
💔 -1 overall
This message was automatically generated. |
|
@wchevreuil I agree with you. |
|
🎊 +1 overall
This message was automatically generated. |
sunhelly
commented
Jul 3, 2019
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@wchevreuil I have updated the patch, please help to review. |
|
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.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@wchevreuil I have replied on the remaining comments, and updated my patch again. |
wchevreuil
reviewed
Jul 10, 2019
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.
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Show resolved
Hide resolved
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
|
@wchevreuil I have updated the codes as replied in all the comments. Please review again. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Jul 14, 2019
| Pair<ServerName, RegionStateNode> gotPair = createTableAndSetARegionState(newGroup, 5); | ||
| try{ | ||
| rsGroupAdmin.moveTables(Sets.newHashSet(tableName), newGroup.getName()); | ||
| fail("move tables to group should fail"); |
There was a problem hiding this comment.
Small nit: let's mention the test expected a IOE that didn't happen.
There was a problem hiding this comment.
Thanks, I'll change the fail message to mention the expected exception.
wchevreuil
reviewed
Jul 14, 2019
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
wchevreuil
reviewed
Jul 14, 2019
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin2.java
Outdated
Show resolved
Hide resolved
|
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. |
|
@wchevreuil Thanks for your comments, I have updated the patch. |
|
Squashed commits locally, then pushed to master. Closing this PR now, thanks for contributing @sunhelly ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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