HBASE-24560 Add a new option of designatedfile in RegionMover #1901
Conversation
This file contains 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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Jun 18, 2020
| /** | ||
| * Set the designated file. Designated file contains hostnames where region moves. Designated file should | ||
| * have 'host:port' per line. Port is mandatory here as we can have many RS running on a single | ||
| * host. |
There was a problem hiding this comment.
Can you please bring these comment lines within 100 chars. Somehow build is not failing for 100+ char space usage on one line.
| @@ -413,7 +429,7 @@ private void loadRegions(List<RegionInfo> regionsToMove) | |||
| * Unload regions from given {@link #hostname} using ack/noAck mode and {@link #maxthreads}.In | |||
| * noAck mode we do not make sure that region is successfully online on the target region | |||
| * server,hence it is best effort.We do not unload regions to hostnames given in | |||
| * {@link #excludeFile}. | |||
| * {@link #excludeFile}. We only unload regions to hostnames given in {@link #designatedFile}. | |||
There was a problem hiding this comment.
Please put the condition:
If designatedFile is present with some contents, we will unload regions to hostnames provided in {@link #designatedFile}
| @@ -435,6 +451,10 @@ public boolean unload() throws InterruptedException, ExecutionException, Timeout | |||
| LOG.debug("List of region servers: {}", regionServers); | |||
| return false; | |||
| } | |||
| // Remove RS present not in the designated file | |||
There was a problem hiding this comment.
nit: Remove RS not present in the designated file
|
|
||
| private void filterDesignatedServers(List<ServerName> onlineServers) { | ||
| List<String> designatedServers = readServersFromFile(designatedFile); | ||
| if (designatedServers == null || designatedServers.isEmpty()) { |
There was a problem hiding this comment.
designatedServers == null is redundant
| .forEach(excludeServers::add); | ||
| Files.readAllLines(Paths.get(filename)).stream().map(String::trim) | ||
| .filter(((Predicate<String>) String::isEmpty).negate()).map(String::toLowerCase) | ||
| .forEach(servers::add); | ||
| } catch (IOException e) { | ||
| LOG.warn("Exception while reading excludes file, continuing anyways", e); |
There was a problem hiding this comment.
We should not continue if file read fails right?
| @@ -435,6 +451,10 @@ public boolean unload() throws InterruptedException, ExecutionException, Timeout | |||
| LOG.debug("List of region servers: {}", regionServers); | |||
| return false; | |||
| } | |||
| // Remove RS present not in the designated file | |||
| if (designatedFile != null) { | |||
| filterDesignatedServers(regionServers); | |||
There was a problem hiding this comment.
We should use stripExcludes() for both purpose: exclude file and designated file.
Add filename as argument in stripExcludes().
There was a problem hiding this comment.
I haven't thought of how to elegantly combine excludeFile and designedFile in one method. Could you please give some advice?
There was a problem hiding this comment.
We can combine filterDesignatedServers() and stripExcludes().
If you see carefully, both are doing the same thing except for one is excluding what is present in file and another is including. So whether to include/exclude all RS present in given file is something you can provide as boolean argument and act accordingly.
Something like:
private void includeExcludeRegionServers(String fileName, List<ServerName> regionServers, boolean isInclude)
For, filterDesignatedServers(), it can be:
includeExcludeRegionServers(designatedFile, onlineServers, true);
and stripExcludes can be replaced with:
includeExcludeRegionServers(excludeFile, regionServers, false);
Sounds good?
There was a problem hiding this comment.
Done as you said. Thanks for your advice.
| */ | ||
| private List<String> readExcludes(String excludeFile) throws IOException { | ||
| List<String> excludeServers = new ArrayList<>(); | ||
| if (excludeFile == null) { |
There was a problem hiding this comment.
Please use inverted logic (improves readability):
if (excludeFile != null){
...
...
...
}
return excludeServers;
| RegionMoverBuilder rmBuilder = new RegionMoverBuilder(rs, TEST_UTIL.getConfiguration()) | ||
| .designatedFile(designatedFile.getCanonicalPath()); | ||
| try (RegionMover rm = rmBuilder.build()) { | ||
| LOG.info("Unloading {} regions", rs); |
There was a problem hiding this comment.
Use debug log for test cases :)
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
reviewed
Jun 21, 2020
| @@ -413,7 +429,7 @@ private void loadRegions(List<RegionInfo> regionsToMove) | |||
| * Unload regions from given {@link #hostname} using ack/noAck mode and {@link #maxthreads}.In | |||
| * noAck mode we do not make sure that region is successfully online on the target region | |||
| * server,hence it is best effort.We do not unload regions to hostnames given in | |||
| * {@link #excludeFile}. | |||
| * {@link #excludeFile}. We only unload regions to hostnames given in {@link #designatedFile}. | |||
There was a problem hiding this comment.
Say it if this param is provided. This is anyways an optional stuff
| private void filterDesignatedServers(List<ServerName> onlineServers) { | ||
| List<String> designatedServers = readServersFromFile(designatedFile); | ||
| if (designatedServers == null || designatedServers.isEmpty()) { | ||
| LOG.warn("Designated server is null, use all online server."); |
There was a problem hiding this comment.
Pls make the log more meaningful.. We can say no designated servers provided in the file + this.designatedFile
|
🎊 +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. |
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
Ping @anoopsjohn @virajjasani |
virajjasani
reviewed
Jun 29, 2020
| } | ||
|
|
||
| /** | ||
| * Excludes the servername whose hostname and port portion matches the list given in exclude file | ||
| * Designates or excludes the servername whose hostname and port portion matches the list given | ||
| * in the file. |
There was a problem hiding this comment.
We can give examples of both designatedFile and excludeFile and how it works with isInclude param specifically.
I want to add designatedFile and excludeFile options to the graceful_stop.sh @virajjasani |
virajjasani
approved these changes
Jun 30, 2020
| * Example:<br> | ||
| * If you want to designated RSs, suppose designatedFile has RS1, regionServers has RS1, RS2 and | ||
| * RS3. When call includeExcludeRegionServers(designatedFile, regionServers, true), regionServers | ||
| * left RS1. |
There was a problem hiding this comment.
nit: When we call includeExcludeRegionServers(designatedFile, regionServers, true), RS2 and RS3 are removed from regionServers list so that regions can move to only RS1.
| * left RS1. | ||
| * If you want to exclude RSs, suppose excludeFile has RS1, regionServers has RS1, RS2 and RS3. | ||
| * When call includeExcludeRegionServers(excludeFile, servers, false), regionServers left RS2 and | ||
| * RS3. |
There was a problem hiding this comment.
nit: When we call includeExcludeRegionServers(excludeFile, servers, false), RS1 is removed from regionServers list so that regions can move to only RS2 and RS3.
There was a problem hiding this comment.
done. Thank you @virajjasani
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
approved these changes
Jul 1, 2020
|
@anoopsjohn would you like to take a look? |
anoopsjohn
approved these changes
Jul 1, 2020
virajjasani
pushed a commit
that referenced
this pull request
Jul 1, 2020
Closes #1901 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
Closes apache#1901 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
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.
HBASE-24560