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 3 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
47 changes: 43 additions & 4 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,19 @@ 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 version and the new value of
* "hbase.min.version.move.system.tables" does not trigger any region movement.
* It is assumed that the configured range of versions do not require special
* handling of moving system table regions to higher versioned RegionServer.
*/
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 +371,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 @@ -2551,19 +2566,44 @@ private int setOfflineInZooKeeper(final RegionState state, final ServerName dest
* 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);
}

/**
* 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.
* 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.
* @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 +2613,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 +2734,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