You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/06/29 10:07:12 UTC
[GitHub] [hbase] virajjasani opened a new pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
virajjasani opened a new pull request #3438:
URL: https://github.com/apache/hbase/pull/3438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
bharathv commented on pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#issuecomment-870979844
Also, the "localhost" part is suspcicious, why does AM look for a localhost address?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r661585320
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -253,6 +253,28 @@
// are persisted in meta with a state store
private final RegionStateStore regionStateStore;
+ /**
+ * Min version to consider for moving system tables regions to higher
+ * versioned RS. If RS has higher version than rest of the cluster but that
+ * version is less than this value, we should not move system table regions
+ * to that RS. If RS has higher version than rest of the cluster but that
+ * version is greater than or equal to this value, we should move system
+ * table regions to that RS. This is optional config and default value is
+ * empty string ({@link #DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG}).
+ * For instance, if we do not want meta region to be moved to RS with higher
+ * version until that version is >= 2.0.0, then we can configure
+ * "hbase.min.version.move.system.tables" as "2.0.0".
+ * When operator uses this config, it should be used with care, meaning
+ * we should be confident that even if user table regions come to RS with
+ * higher version (that rest of cluster), it would not cause any
Review comment:
@apurtell Created master branch PR #3439 yesterday. Will create branch-2 PR as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r662179581
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -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)) {
Review comment:
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 :) )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r661697488
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -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)) {
Review comment:
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) {
,,,,
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r662179581
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -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)) {
Review comment:
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:
```
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 :) )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#issuecomment-871762414
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 15m 10s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -0 :warning: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 10m 23s | branch-1 passed |
| +1 :green_heart: | compile | 0m 52s | branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | compile | 0m 52s | branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | checkstyle | 2m 3s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 24s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 48s | branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javadoc | 0m 43s | branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +0 :ok: | spotbugs | 3m 16s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 3m 12s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 2s | the patch passed |
| +1 :green_heart: | compile | 0m 41s | the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javac | 0m 41s | the patch passed |
| +1 :green_heart: | compile | 0m 47s | the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | javac | 0m 47s | the patch passed |
| -1 :x: | checkstyle | 1m 50s | hbase-server: The patch generated 4 new + 231 unchanged - 0 fixed = 235 total (was 231) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 3m 6s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 4m 46s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 32s | the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javadoc | 0m 43s | the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | findbugs | 3m 7s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 145m 55s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 30s | The patch does not generate ASF License warnings. |
| | | 205m 16s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
| | hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3438 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 774aa1b1dcb2 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3438/out/precommit/personality/provided.sh |
| git revision | branch-1 / 28f36f4 |
| Default Java | Azul Systems, Inc.-1.7.0_272-b10 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/3/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/3/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/3/testReport/ |
| Max. process+thread count | 4488 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/3/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r661023484
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2564,6 +2588,45 @@ public int compare(Pair<ServerName, String> o1, Pair<ServerName, String> o2) {
return res;
}
+ /**
+ * 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.
+ * This method is same as {@link #getExcludedServersForSystemTable()} with
+ * the only difference is we can disable this exclusion using config:
+ * "hbase.min.version.move.system.tables".
+ *
+ * @return List of Excluded servers for System table regions.
+ */
+ private List<ServerName> getExcludedServersForSystemTableUnlessAllowed() {
Review comment:
Can we club the logic of both the methods and pass an optional boolean for version check?
The two functions that look very much alike except for the tail part.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#issuecomment-871500621
> Also, the "localhost" part is suspicious, why does AM look for a localhost address?
It is `BOGUS_SERVER_NAME`. AM tries to put it in `FAILED_OPEN` state after getting one:
```
private void processBogusAssignments(Map<ServerName, List<HRegionInfo>> bulkPlan) {
if (bulkPlan.containsKey(LoadBalancer.BOGUS_SERVER_NAME)) {
// Found no plan for some regions, put those regions in RIT
for (HRegionInfo hri : bulkPlan.get(LoadBalancer.BOGUS_SERVER_NAME)) {
regionStates.updateRegionState(hri, State.FAILED_OPEN);
}
bulkPlan.remove(LoadBalancer.BOGUS_SERVER_NAME);
}
}
```
And it is `RSGroupBasedLoadBalancer` that tries to add it:
```
//if not server is available assign to bogus so it ends up in RIT
if(!assignments.containsKey(LoadBalancer.BOGUS_SERVER_NAME)) {
assignments.put(LoadBalancer.BOGUS_SERVER_NAME, new ArrayList<HRegionInfo>());
}
assignments.get(LoadBalancer.BOGUS_SERVER_NAME).add(region);
```
and similar logic is available few other places in `RSGroupBasedLoadBalancer`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r661697488
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -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)) {
Review comment:
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) {
,,,,
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r661684889
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -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)) {
Review comment:
super nit: club the conditions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r660994432
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -253,6 +253,28 @@
// are persisted in meta with a state store
private final RegionStateStore regionStateStore;
+ /**
+ * Min version to consider for moving system tables regions to higher
+ * versioned RS. If RS has higher version than rest of the cluster but that
+ * version is less than this value, we should not move system table regions
+ * to that RS. If RS has higher version than rest of the cluster but that
+ * version is greater than or equal to this value, we should move system
+ * table regions to that RS. This is optional config and default value is
+ * empty string ({@link #DEFAULT_MIN_VERSION_MOVE_SYS_TABLES_CONFIG}).
+ * For instance, if we do not want meta region to be moved to RS with higher
+ * version until that version is >= 2.0.0, then we can configure
+ * "hbase.min.version.move.system.tables" as "2.0.0".
+ * When operator uses this config, it should be used with care, meaning
+ * we should be confident that even if user table regions come to RS with
+ * higher version (that rest of cluster), it would not cause any
Review comment:
The language here is ambiguous. Better to say something like
> 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 the configured range of versions do not require special handling.
This should also be committed to all branches, not just branch-1, for consistent functionality across all future releasing versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv edited a comment on pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
bharathv edited a comment on pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#issuecomment-870979844
Also, the "localhost" part is suspicious, why does AM look for a localhost address?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#discussion_r661689913
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -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)) {
Review comment:
Since the argument `checkForMinVersion` controls this specific flow, I thought keeping it this way would make it more readable. Thought?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani merged pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
virajjasani merged pull request #3438:
URL: https://github.com/apache/hbase/pull/3438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#issuecomment-871646080
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 11m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -0 :warning: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 10m 54s | branch-1 passed |
| +1 :green_heart: | compile | 0m 42s | branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | compile | 0m 45s | branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | checkstyle | 1m 55s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 21s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 49s | branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javadoc | 0m 42s | branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +0 :ok: | spotbugs | 3m 16s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 3m 12s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 2s | the patch passed |
| +1 :green_heart: | compile | 0m 42s | the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javac | 0m 42s | the patch passed |
| +1 :green_heart: | compile | 0m 47s | the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | javac | 0m 47s | the patch passed |
| -1 :x: | checkstyle | 1m 48s | hbase-server: The patch generated 4 new + 231 unchanged - 0 fixed = 235 total (was 231) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 3m 9s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 5m 0s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 32s | the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javadoc | 0m 43s | the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | findbugs | 3m 9s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 155m 4s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 32s | The patch does not generate ASF License warnings. |
| | | 210m 58s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3438 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux eddc05d43a0b 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-agent/workspace/Base-PreCommit-GitHub-PR_PR-3438/out/precommit/personality/provided.sh |
| git revision | branch-1 / 28f36f4 |
| Default Java | Azul Systems, Inc.-1.7.0_272-b10 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/2/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/2/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/2/testReport/ |
| Max. process+thread count | 5023 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/2/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3438: HBASE-22923 min version of RegionServer to move system table regions
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3438:
URL: https://github.com/apache/hbase/pull/3438#issuecomment-870622358
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 11m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -0 :warning: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 9m 40s | branch-1 passed |
| +1 :green_heart: | compile | 0m 40s | branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | compile | 0m 44s | branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | checkstyle | 1m 49s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 5s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 49s | branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javadoc | 0m 41s | branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +0 :ok: | spotbugs | 3m 5s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 3m 2s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 50s | the patch passed |
| +1 :green_heart: | compile | 0m 41s | the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javac | 0m 41s | the patch passed |
| +1 :green_heart: | compile | 0m 44s | the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | javac | 0m 44s | the patch passed |
| -1 :x: | checkstyle | 1m 37s | hbase-server: The patch generated 4 new + 228 unchanged - 0 fixed = 232 total (was 228) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 2m 50s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 4m 26s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 32s | the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19 |
| +1 :green_heart: | javadoc | 0m 41s | the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10 |
| +1 :green_heart: | findbugs | 2m 55s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 174m 49s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 40s | The patch does not generate ASF License warnings. |
| | | 227m 24s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
| | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
| | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3438 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 629130e8e8bc 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3438/out/precommit/personality/provided.sh |
| git revision | branch-1 / 395eb0c |
| Default Java | Azul Systems, Inc.-1.7.0_272-b10 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/1/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/1/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/1/testReport/ |
| Max. process+thread count | 4214 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3438/1/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org