-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-22923 min version of RegionServer to move system table regions #3439
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 2 commits
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 |
|---|---|---|
|
|
@@ -163,6 +163,19 @@ public class AssignmentManager { | |
| private final RegionStates regionStates = new RegionStates(); | ||
| private final RegionStateStore regionStateStore; | ||
|
|
||
| /** | ||
| * When the operator uses this configuration option, any version between | ||
| * the current version and the new value of | ||
| * "hbase.min.version.move.system.tables" does not trigger any region movement. | ||
|
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. Do you want to explain what the 'region movement' is here? The auto-migration of system tables to newer server versions. A dev reading this w/o context might not tie the 'reigon movement' to the auto-movement to newer versions. |
||
| * It is assumed that the configured range of versions do not require special | ||
| * handling of moving system table regions to higher versioned RegionServer. | ||
|
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. s/do/does/ and s/of//. Perhaps point at the location in the code where this is done. Can we add examples of how this would work -- the ones from the JIRA?
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. Done, Thanks |
||
| */ | ||
| private final String minVersionToMoveSysTables; | ||
|
|
||
| private static final String MIN_VERSION_MOVE_SYS_TABLES_CONFIG = | ||
| "hbase.min.version.move.system.tables"; | ||
| private static final String DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG = ""; | ||
|
|
||
| private final Map<ServerName, Set<byte[]>> rsReports = new HashMap<>(); | ||
|
|
||
| private final boolean shouldAssignRegionsWithFavoredNodes; | ||
|
|
@@ -211,6 +224,8 @@ public AssignmentManager(final MasterServices master) { | |
| } else { | ||
| this.deadMetricChore = null; | ||
| } | ||
| minVersionToMoveSysTables = conf.get(MIN_VERSION_MOVE_SYS_TABLES_CONFIG, | ||
| DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG); | ||
| } | ||
|
|
||
| public void start() throws IOException, KeeperException { | ||
|
|
@@ -550,7 +565,7 @@ public void checkIfShouldMoveSystemRegionAsync() { | |
| List<RegionPlan> plans = new ArrayList<>(); | ||
| // TODO: I don't think this code does a good job if all servers in cluster have same | ||
| // version. It looks like it will schedule unnecessary moves. | ||
| for (ServerName server : getExcludedServersForSystemTable()) { | ||
| for (ServerName server : getExcludedServersForSystemTable(true)) { | ||
| if (master.getServerManager().isServerDead(server)) { | ||
| // TODO: See HBASE-18494 and HBASE-18495. Though getExcludedServersForSystemTable() | ||
| // considers only online servers, the server could be queued for dead server | ||
|
|
@@ -2280,24 +2295,48 @@ private void addToPendingAssignment(final HashMap<RegionInfo, RegionStateNode> r | |
| * For system tables, we must assign them to a server with highest version. | ||
| */ | ||
| public List<ServerName> getExcludedServersForSystemTable() { | ||
|
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 used?
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. Yes, this is used by HMaster and AM both. |
||
| return getExcludedServersForSystemTable(false); | ||
| } | ||
|
|
||
| /** | ||
| * Get a list of servers that this region can not assign to. | ||
|
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. There doesn't seem to be a 'region' passed to this method.... So is it any system table region? |
||
| * For system table, we must assign them to a server with highest version. | ||
| * We can disable this exclusion using config: | ||
| * "hbase.min.version.move.system.tables" if checkForMinVersion is true. | ||
| * | ||
| * @param checkForMinVersion if true, check for minVersionToMoveSysTables | ||
| * and decide moving system table regions accordingly. | ||
|
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. This method doesn't do move? It just looks for servers to exclude.... |
||
| * @return List of Excluded servers for System table regions. | ||
| */ | ||
| private List<ServerName> getExcludedServersForSystemTable( | ||
| boolean checkForMinVersion) { | ||
| // TODO: This should be a cached list kept by the ServerManager rather than calculated on each | ||
| // move or system region assign. The RegionServerTracker keeps list of online Servers with | ||
| // RegionServerInfo that includes Version. | ||
| List<Pair<ServerName, String>> serverList = master.getServerManager().getOnlineServersList() | ||
| .stream() | ||
| .map((s)->new Pair<>(s, master.getRegionServerVersion(s))) | ||
| .collect(Collectors.toList()); | ||
| .stream() | ||
| .map(s->new Pair<>(s, master.getRegionServerVersion(s))) | ||
| .collect(Collectors.toList()); | ||
| if (serverList.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
| String highestVersion = Collections.max(serverList, | ||
| (o1, o2) -> VersionInfo.compareVersion(o1.getSecond(), o2.getSecond())).getSecond(); | ||
| if (checkForMinVersion) { | ||
| if (!DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG.equals(minVersionToMoveSysTables)) { | ||
| int comparedValue = VersionInfo.compareVersion(minVersionToMoveSysTables, highestVersion); | ||
| if (comparedValue > 0) { | ||
| return Collections.emptyList(); | ||
| } | ||
| } | ||
| } | ||
| return serverList.stream() | ||
| .filter((p)->!p.getSecond().equals(highestVersion)) | ||
| .map(Pair::getFirst) | ||
| .collect(Collectors.toList()); | ||
| .filter(pair -> !pair.getSecond().equals(highestVersion)) | ||
| .map(Pair::getFirst) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
|
|
||
| MasterServices getMaster() { | ||
| return master; | ||
| } | ||
|
|
||
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.
Is it 'new value' or just 'value'.