Skip to content

HBASE-22923 min version of RegionServer to move system table regions #3438

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.

Sign up for GitHub

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

Merged
virajjasani merged 5 commits into from
Jul 1, 2021
Merged

HBASE-22923 min version of RegionServer to move system table regions #3438

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b7fbcf
HBASE-22923 min version of RegionServer to move system table regions
virajjasani Jun 29, 2021
43e7584
Merge branch 'branch-1' of github.com:apache/hbase into HBASE-22923-b…
virajjasani Jun 30, 2021
0cc95dd
addressing comments by @apurtell and @bharathv
virajjasani Jun 30, 2021
b7ec7e0
addressing comments by @saintstack
virajjasani Jun 30, 2021
9986bfa
addendum
virajjasani Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 65 additions & 6 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,31 @@ public class AssignmentManager extends ZooKeeperListener {
// are persisted in meta with a state store
private final RegionStateStore regionStateStore;

/**
* When the operator uses this configuration option, any version between
* the current cluster version and the value of "hbase.min.version.move.system.tables"
* does not trigger any auto-region movement. Auto-region movement here
* refers to auto-migration of system table regions to newer server versions.
* It is assumed that the configured range of versions does not require special
* handling of moving system table regions to higher versioned RegionServer.
* This auto-migration is done by {@link #checkIfShouldMoveSystemRegionAsync()}.
* Example: Let's assume the cluster is on version 1.4.0 and we have
* set "hbase.min.version.move.system.tables" as "2.0.0". Now if we upgrade
* one RegionServer on 1.4.0 cluster to 1.6.0 (< 2.0.0), then AssignmentManager will
* not move hbase:meta, hbase:namespace and other system table regions
* to newly brought up RegionServer 1.6.0 as part of auto-migration.
* However, if we upgrade one RegionServer on 1.4.0 cluster to 2.2.0 (> 2.0.0),
* then AssignmentManager will move all system table regions to newly brought
* up RegionServer 2.2.0 as part of auto-migration done by
* {@link #checkIfShouldMoveSystemRegionAsync()}.
* "hbase.min.version.move.system.tables" is introduced as part of HBASE-22923.
*/
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 = "";

/**
* For testing only! Set to true to skip handling of split and merge.
*/
Expand Down Expand Up @@ -358,6 +383,8 @@ public AssignmentManager(MasterServices server, ServerManager serverManager,
scheduledThreadPoolExecutor.scheduleWithFixedDelay(new FailedOpenRetryRunnable(),
failedOpenRetryPeriod, failedOpenRetryPeriod, TimeUnit.MILLISECONDS);
}
minVersionToMoveSysTables = conf.get(MIN_VERSION_MOVE_SYS_TABLES_CONFIG,
DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG);
}

/**
Expand Down Expand Up @@ -2545,25 +2572,58 @@ private int setOfflineInZooKeeper(final RegionState state, final ServerName dest
}

/**
* Get a list of servers that this region can not assign to.
* For system table, we must assign them to a server with highest version.
* For a given cluster with mixed versions of servers, get a list of
* servers with lower versions, where system table regions should not be
* assigned to.
* For system table, we must assign regions to a server with highest version.
* RS will report to master before register on zk, and only when RS have registered on zk we can
* know the version. So in fact we will never assign a system region to a RS without registering on zk.
*/
public List<ServerName> getExcludedServersForSystemTable() {
return getExcludedServersForSystemTable(false);
}

/**
* For a given cluster with mixed versions of servers, get a list of
* servers with lower versions, where system table regions should not be
* assigned to.
* For system table, we must assign regions to a server with highest version.
* However, we can disable this exclusion using config:
* "hbase.min.version.move.system.tables" if checkForMinVersion is true.
* Detailed explanation available with definition of minVersionToMoveSysTables.
*
* @param checkForMinVersion If false, return a list of servers with lower version. If true,
* compare higher version with minVersionToMoveSysTables. Only if higher version is greater
* than minVersionToMoveSysTables, this method returns list of servers with lower version. If
* higher version is less than or equal to minVersionToMoveSysTables, returns empty list.
* An example is provided with definition of minVersionToMoveSysTables.
* @return List of Excluded servers for System table regions.
*/
private List<ServerName> getExcludedServersForSystemTable(
boolean checkForMinVersion) {
List<Pair<ServerName, String>> serverList = new ArrayList<>();
for (ServerName s : serverManager.getOnlineServersList()) {
serverList.add(new Pair<>(s, server.getRegionServerVersion(s)));
}
if (serverList.isEmpty()) {
return new ArrayList<>();
return Collections.emptyList();
}
String highestVersion = Collections.max(serverList, new Comparator<Pair<ServerName, String>>() {
String highestVersion = Collections.max(serverList,
new Comparator<Pair<ServerName, String>>() {
@Override
public int compare(Pair<ServerName, String> o1, Pair<ServerName, String> o2) {
return VersionInfo.compareVersion(o1.getSecond(), o2.getSecond());
}
}).getSecond();
if (checkForMinVersion) {
if (!DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG.equals(minVersionToMoveSysTables)) {
Comment on lines +2618 to +2619
Copy link
Contributor

@bharathv bharathv Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: club the conditions

Copy link
Contributor Author

@virajjasani virajjasani Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the argument checkForMinVersion controls this specific flow, I thought keeping it this way would make it more readable. Thought?

Copy link
Contributor

@bharathv bharathv Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion. If you ask me, I like the following version but its subjective. I'm fine with whatever you think is good.

checkForMinVersion = checkForMinVersion && !DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG.equals(minVersionToMoveSysTables);
if (checkForMinVersion) {
  ,,,,
}

Copy link
Contributor Author

@virajjasani virajjasani Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love these discussions, always learn a thing or two :)

decisionFactor = decisionFactor && additionalFactors yeah, this is also nice way. I wish we had some standards around this.
For now, let me keep it as is as you don't have strong opinion, I still find this simpler from readability viewpoint:

  if (decisionFactor) {
    if (additionalFactors) {
    }
  }

I think this is why we don't have standards because individuals find different approaches as simpler ones than others (but CPU doesn't care :) )

int comparedValue = VersionInfo
.compareVersion(minVersionToMoveSysTables, highestVersion);
if (comparedValue > 0) {
return Collections.emptyList();
}
}
}
List<ServerName> res = new ArrayList<>();
for (Pair<ServerName, String> pair : serverList) {
if (!pair.getSecond().equals(highestVersion)) {
Expand All @@ -2573,7 +2633,6 @@ public int compare(Pair<ServerName, String> o1, Pair<ServerName, String> o2) {
return res;
}


/**
* @param region the region to assign
* @return Plan for passed <code>region</code> (If none currently, it creates one or
Expand Down Expand Up @@ -2695,7 +2754,7 @@ public void run() {
synchronized (checkIfShouldMoveSystemRegionLock) {
// RS register on ZK after reports startup on master
List<HRegionInfo> regionsShouldMove = new ArrayList<>();
for (ServerName server : getExcludedServersForSystemTable()) {
for (ServerName server : getExcludedServersForSystemTable(true)) {
regionsShouldMove.addAll(getCarryingSystemTables(server));
}
if (!regionsShouldMove.isEmpty()) {
Expand Down