HBASE-24418 Consolidate Normalizer implementations #1786

ndimiduk · 2020-05-27T00:47:34Z

Simplify our Normalizer story to have just a single, configurable
implementation.

fold the features of MergeNormalizer into
SimpleRegionNormalizer, removing the intermediate abstract class.
configuration keys for merge-only features now share a common
structure.
add configuration to selectively disable normalizer split/merge
operations.
RegionNormalizer now extends Configurable instead of creating a
new instance of HBaseConfiguration or snooping one off of other
fields.
avoid the extra RPCs by using MasterServices instead of
MasterRpcServices.
boost test coverage of all the various flags and feature
combinations.

ndimiduk · 2020-05-27T00:51:56Z

I intend to backport this at least to branch-2.3. I think branch-2.2 needs some other patches before this would apply.

PTAL, @saintstack @Apache9 @infraio @huaxiangsun @joshelser @mnpoonia @ddupg.

Apache-HBase · 2020-05-27T01:28:02Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 30s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+1 💚	mvninstall	3m 45s	master passed
+1 💚	checkstyle	1m 9s	master passed
+1 💚	spotbugs	2m 0s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	3m 21s	the patch passed
-0 ⚠️	checkstyle	1m 5s	hbase-server: The patch generated 1 new + 100 unchanged - 1 fixed = 101 total (was 101)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	hadoopcheck	11m 14s	Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚	spotbugs	2m 7s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 14s	The patch does not generate ASF License warnings.
		32m 44s

Subsystem	Report/Notes
Docker	Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle
uname	Linux 624555381f8e 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `a9205f8`
checkstyle	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt
Max. process+thread count	94 (vs. ulimit of 12500)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-05-27T03:48:50Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 30s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+1 💚	mvninstall	3m 50s	master passed
+1 💚	compile	0m 54s	master passed
+1 💚	shadedjars	5m 37s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 39s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	3m 24s	the patch passed
+1 💚	compile	0m 54s	the patch passed
+1 💚	javac	0m 54s	the patch passed
+1 💚	shadedjars	5m 34s	patch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 37s	hbase-server generated 25 new + 1 unchanged - 0 fixed = 26 total (was 1)
		_ Other Tests _
+1 💚	unit	149m 32s	hbase-server in the patch passed.
		173m 31s

Subsystem	Report/Notes
Docker	Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 30d0d2b03097 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `a9205f8`
Default Java	1.8.0_232
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-jdk8-hadoop3-check/output/diff-javadoc-javadoc-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/testReport/
Max. process+thread count	4671 (vs. ulimit of 12500)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-05-27T04:34:55Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 25s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+1 💚	mvninstall	4m 42s	master passed
+1 💚	compile	1m 8s	master passed
+1 💚	shadedjars	6m 21s	branch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 42s	hbase-server in master failed.
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 24s	the patch passed
+1 💚	compile	1m 9s	the patch passed
+1 💚	javac	1m 9s	the patch passed
+1 💚	shadedjars	6m 20s	patch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 40s	hbase-server in the patch failed.
		_ Other Tests _
+1 💚	unit	192m 0s	hbase-server in the patch passed.
		219m 41s

Subsystem	Report/Notes
Docker	Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 9505a86dd57e 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `a9205f8`
Default Java	2020-01-14
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/testReport/
Max. process+thread count	3266 (vs. ulimit of 12500)
modules	C: hbase-server U: hbase-server
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/1/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

virajjasani

Left few comments. Looks good overall with much better consolidation.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/RegionNormalizer.java

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

...rver/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

virajjasani · 2020-05-27T14:28:48Z

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

-        masterServices.getAssignmentManager().getRegionStates().getRegionsOfTable(table);
+    List<RegionInfo> tableRegions = masterServices.getAssignmentManager()
+      .getRegionStates()
+      .getRegionsOfTable(table);


We retrieve List<RegionInfo> tableRegions again in computeSplitNormalizationPlans() and computeMergeNormalizationPlans(). It should be safe to pass this list to both methods and avoid calls to AM?

+1 on above

Yeah, let me see if i can do something better here. I don't like how accessing these collection objects doesn't align with their use here.

This is all too messy for my liking. These computeSplitNormalizationPlans and computeMergeNormalizationPlans share a bunch of data in common, and both go rummaging around in the AM independently. TableName, Region list, RegionStates, computed average size, it could all be reused...

How much do you guys care about this? The best solution i have right now is an inner class that does the lookups once, and holds state across methods.

The inner class ends up being awkward too. Let me address the other feedback and return to this one.

I've pulled out the common state into an inner class; it's not so bad after all. PTAL.

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

saintstack

Looks great. Generally, I think the default is too aggressive. Also think should purge the notion that there could be alternate normalizer impls... Save a bit on config and helps simplify.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

saintstack · 2020-05-27T16:35:48Z

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

-        masterServices.getAssignmentManager().getRegionStates().getRegionsOfTable(table);
+    List<RegionInfo> tableRegions = masterServices.getAssignmentManager()
+      .getRegionStates()
+      .getRegionsOfTable(table);


+1 on above

...server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SplitPlanFirstComparator.java

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

huaxiangsun

+1, nice change!
Only the test testNormalizerCannotMergeNonAdjacentRegions() needs to be fixed, as without the patch, it also passed for me.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

...server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SplitPlanFirstComparator.java

...rver/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java

huaxiangsun · 2020-05-27T21:28:48Z

diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java
index 6b75ed7881..eeb9bef0c8 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java
@@ -396,13 +396,13 @@ public class TestSimpleRegionNormalizer {
     final byte[][] keys = {
       null,
       Bytes.toBytes("aa"),
-      Bytes.toBytes("aa1"),
       Bytes.toBytes("aa1!"),
+      Bytes.toBytes("aa1"),
       Bytes.toBytes("aa2"),
       null,
     };
     final List<RegionInfo> regionInfos = createRegionInfos(tableName, keys);
-    final Map<byte[], Integer> regionSizes = createRegionSizesMap(regionInfos, 3, 1, 3, 1, 5);
+    final Map<byte[], Integer> regionSizes = createRegionSizesMap(regionInfos, 3, 1, 1, 3, 5);
     setupMocksForNormalizer(regionSizes, regionInfos);
 
     // Compute the plan, no merge plan returned as they are not adjacent.

ndimiduk · 2020-05-27T21:47:59Z

Generally, I think the default is too aggressive.

@saintstack Looks liked we originally ran the normalizer on a 30min period. This was reduced to 5 min for 2.0, according to https://hbase.apache.org/book.html#upgrade2.0.changed.defaults

saintstack · 2020-05-27T21:54:25Z

Interesting. Wonder why the change was made.

ndimiduk · 2020-05-29T00:30:46Z

Interesting. Wonder why the change was made.

Looks like it was slipped in with https://issues.apache.org/jira/browse/HBASE-15065. There's some interesting comments as well from the original author in regard to the plan order, the move from a single global, optimized plan to the current list of actions ("plans").

Apache-HBase · 2020-05-29T21:53:33Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 24s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+0 🆗	mvndep	0m 29s	Maven dependency ordering for branch
+1 💚	mvninstall	4m 44s	master passed
+1 💚	checkstyle	2m 42s	master passed
+0 🆗	refguide	6m 33s	branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	spotbugs	16m 19s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s	Maven dependency ordering for patch
+1 💚	mvninstall	4m 43s	the patch passed
+1 💚	checkstyle	2m 40s	root: The patch generated 0 new + 100 unchanged - 1 fixed = 100 total (was 101)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 3s	The patch has no ill-formed XML file.
+0 🆗	refguide	7m 14s	patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	hadoopcheck	14m 25s	Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚	spotbugs	16m 7s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 44s	The patch does not generate ASF License warnings.
		87m 53s

Subsystem	Report/Notes
Docker	Client=19.03.10 Server=19.03.10 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	dupname asflicense refguide xml spotbugs hadoopcheck hbaseanti checkstyle
uname	Linux c8a923790ccb 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `06949ff`
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-general-check/output/branch-site/book.html
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-general-check/output/patch-site/book.html
Max. process+thread count	123 (vs. ulimit of 12500)
modules	C: hbase-common hbase-server . U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-05-30T01:38:29Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 42s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 23s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 16s	master passed
+1 💚	compile	3m 21s	master passed
+1 💚	shadedjars	7m 3s	branch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 18s	root in master failed.
-0 ⚠️	javadoc	0m 18s	hbase-common in master failed.
-0 ⚠️	javadoc	0m 50s	hbase-server in master failed.
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s	Maven dependency ordering for patch
+1 💚	mvninstall	5m 3s	the patch passed
+1 💚	compile	3m 36s	the patch passed
+1 💚	javac	3m 36s	the patch passed
+1 💚	shadedjars	7m 5s	patch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 16s	hbase-common in the patch failed.
-0 ⚠️	javadoc	0m 43s	hbase-server in the patch failed.
-0 ⚠️	javadoc	0m 15s	root in the patch failed.
		_ Other Tests _
+1 💚	unit	273m 8s	root in the patch passed.
		312m 47s

Subsystem	Report/Notes
Docker	Client=19.03.10 Server=19.03.10 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 89047dded43f 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `06949ff`
Default Java	2020-01-14
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-common.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-common.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/testReport/
Max. process+thread count	4855 (vs. ulimit of 12500)
modules	C: hbase-common hbase-server . U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/2/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

huaxiangsun · 2020-06-01T18:01:21Z

+1 for the new diff, looks great!

Apache-HBase · 2020-06-01T23:46:11Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 11s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+0 🆗	mvndep	0m 21s	Maven dependency ordering for branch
+1 💚	mvninstall	3m 56s	master passed
+1 💚	checkstyle	2m 16s	master passed
+0 🆗	refguide	5m 26s	branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	spotbugs	12m 16s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 12s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 42s	the patch passed
+1 💚	checkstyle	2m 16s	root: The patch generated 0 new + 100 unchanged - 1 fixed = 100 total (was 101)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 2s	The patch has no ill-formed XML file.
+0 🆗	refguide	5m 31s	patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚	hadoopcheck	12m 9s	Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚	spotbugs	12m 42s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 35s	The patch does not generate ASF License warnings.
		70m 43s

Subsystem	Report/Notes
Docker	Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	dupname asflicense refguide xml spotbugs hadoopcheck hbaseanti checkstyle
uname	Linux bc1af458458d 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `716702a`
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-general-check/output/branch-site/book.html
refguide	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-general-check/output/patch-site/book.html
Max. process+thread count	122 (vs. ulimit of 12500)
modules	C: hbase-common hbase-server . U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-06-02T03:27:57Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 29s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 33s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 9s	master passed
+1 💚	compile	3m 48s	master passed
+1 💚	shadedjars	7m 36s	branch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 19s	hbase-common in master failed.
-0 ⚠️	javadoc	0m 48s	hbase-server in master failed.
-0 ⚠️	javadoc	0m 15s	root in master failed.
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 14s	Maven dependency ordering for patch
+1 💚	mvninstall	5m 8s	the patch passed
+1 💚	compile	3m 38s	the patch passed
+1 💚	javac	3m 38s	the patch passed
+1 💚	shadedjars	7m 28s	patch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 22s	hbase-common in the patch failed.
-0 ⚠️	javadoc	0m 54s	hbase-server in the patch failed.
-0 ⚠️	javadoc	0m 18s	root in the patch failed.
		_ Other Tests _
+1 💚	unit	253m 14s	root in the patch passed.
		292m 27s

Subsystem	Report/Notes
Docker	Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux f8bd796aaad9 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `716702a`
Default Java	2020-01-14
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-common.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-common.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/testReport/
Max. process+thread count	5092 (vs. ulimit of 12500)
modules	C: hbase-common hbase-server . U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2020-06-02T03:48:22Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 39s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 24s	Maven dependency ordering for branch
+1 💚	mvninstall	3m 37s	master passed
+1 💚	compile	2m 15s	master passed
+1 💚	shadedjars	5m 36s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	3m 0s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 19s	the patch passed
+1 💚	compile	2m 19s	the patch passed
+1 💚	javac	2m 19s	the patch passed
+1 💚	shadedjars	5m 33s	patch has no errors when building our shaded downstream artifacts.
-0 ⚠️	javadoc	0m 34s	hbase-server generated 25 new + 1 unchanged - 0 fixed = 26 total (was 1)
-0 ⚠️	javadoc	1m 59s	root generated 25 new + 88 unchanged - 0 fixed = 113 total (was 88)
		_ Other Tests _
-1 ❌	unit	280m 35s	root in the patch failed.
		312m 53s

Subsystem	Report/Notes
Docker	Client=19.03.11 Server=19.03.11 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#1786
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 2eca36fca414 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `716702a`
Default Java	1.8.0_232
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk8-hadoop3-check/output/diff-javadoc-javadoc-hbase-server.txt
javadoc	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk8-hadoop3-check/output/diff-javadoc-javadoc-root.txt
unit	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-root.txt
Test Results	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/testReport/
Max. process+thread count	4755 (vs. ulimit of 12500)
modules	C: hbase-common hbase-server . U: .
Console output	https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1786/3/console
versions	git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by	Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

huaxiangsun · 2020-06-02T17:35:36Z

+1 on the new udpate.

saintstack

Whoops... old review I didn't push....

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

...server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SplitPlanFirstComparator.java

saintstack

I think there are follow ons here... but this is good to go I'd say.

saintstack · 2020-06-02T17:53:51Z

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

+ *     RegionMetrics)</li>
+ *   <li>For each region R0, if R0 is bigger than S * 2, it is kindly requested to split.</li>
+ *   <li>Otherwise, for the next region in the chain R1, if R0 + R1 is smaller then S, R0 and R1
+ *     are kindly requested to merge.</li>


I said it already that this strikes me as too aggressive. In a follow-up we should make this sloppier? S * (2*) + (2*slop)?

Rather than slop, my recommendation is that we add the concept of a "minimum region size" for deciding when to split. I filed HBASE-24464 for that discussion.

saintstack · 2020-06-02T17:54:22Z

...e-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java

-public class SimpleRegionNormalizer extends AbstractRegionNormalizer {
-
+@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG)
+public class SimpleRegionNormalizer implements RegionNormalizer {


So we kept the Interface?

Simplify our Normalizer story to have just a single, configurable implementation. * fold the features of `MergeNormalizer` into `SimpleRegionNormalizer`, removing the intermediate abstract class. * configuration keys for merge-only features now share a common structure. * add configuration to selectively disable normalizer split/merge operations. * `RegionNormalizer` now extends `Configurable` instead of creating a new instance of `HBaseConfiguration` or snooping one off of other fields. * avoid the extra RPCs by using `MasterServices` instead of `MasterRpcServices`. * boost test coverage of all the various flags and feature combinations. Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: huaxiangsun <huaxiangsun@apache.org>

ndimiduk · 2020-06-02T18:39:00Z

Thanks for review everyone. Good discussion!

virajjasani reviewed May 27, 2020

View reviewed changes

saintstack reviewed May 27, 2020

View reviewed changes

huaxiangsun approved these changes May 27, 2020

View reviewed changes

ndimiduk force-pushed the 24418-consolidate-normalizer branch from 171d1fc to fde0a68 Compare May 29, 2020 20:24

ndimiduk requested review from saintstack, huaxiangsun and virajjasani June 1, 2020 22:27

virajjasani approved these changes Jun 2, 2020

View reviewed changes

huaxiangsun approved these changes Jun 2, 2020

View reviewed changes

saintstack reviewed Jun 2, 2020

View reviewed changes

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java Outdated Show resolved Hide resolved

...server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SplitPlanFirstComparator.java Outdated Show resolved Hide resolved

saintstack approved these changes Jun 2, 2020

View reviewed changes

ndimiduk force-pushed the 24418-consolidate-normalizer branch from c21c8a7 to 8e2558a Compare June 2, 2020 18:38

ndimiduk merged commit cb5c0d2 into apache:master Jun 2, 2020

ndimiduk deleted the 24418-consolidate-normalizer branch June 2, 2020 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HBASE-24418 Consolidate Normalizer implementations #1786

HBASE-24418 Consolidate Normalizer implementations #1786

ndimiduk commented May 27, 2020

ndimiduk commented May 27, 2020

Apache-HBase commented May 27, 2020

Apache-HBase commented May 27, 2020

Apache-HBase commented May 27, 2020

virajjasani left a comment

virajjasani May 27, 2020

saintstack May 27, 2020

ndimiduk May 27, 2020

ndimiduk May 28, 2020

ndimiduk May 28, 2020

ndimiduk May 28, 2020

ndimiduk Jun 1, 2020

saintstack left a comment

saintstack May 27, 2020

huaxiangsun left a comment

huaxiangsun commented May 27, 2020 •

edited

Loading

ndimiduk commented May 27, 2020

saintstack commented May 27, 2020

ndimiduk commented May 29, 2020

Apache-HBase commented May 29, 2020

Apache-HBase commented May 30, 2020

huaxiangsun commented Jun 1, 2020

Apache-HBase commented Jun 1, 2020

Apache-HBase commented Jun 2, 2020

Apache-HBase commented Jun 2, 2020

huaxiangsun commented Jun 2, 2020

saintstack left a comment

saintstack left a comment

saintstack Jun 2, 2020

ndimiduk Jun 2, 2020

saintstack Jun 2, 2020

ndimiduk commented Jun 2, 2020

HBASE-24418 Consolidate Normalizer implementations #1786

HBASE-24418 Consolidate Normalizer implementations #1786

Conversation

ndimiduk commented May 27, 2020

ndimiduk commented May 27, 2020

Apache-HBase commented May 27, 2020

Apache-HBase commented May 27, 2020

Apache-HBase commented May 27, 2020

virajjasani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintstack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaxiangsun left a comment

Choose a reason for hiding this comment

huaxiangsun commented May 27, 2020 • edited Loading

ndimiduk commented May 27, 2020

saintstack commented May 27, 2020

ndimiduk commented May 29, 2020

Apache-HBase commented May 29, 2020

Apache-HBase commented May 30, 2020

huaxiangsun commented Jun 1, 2020

Apache-HBase commented Jun 1, 2020

Apache-HBase commented Jun 2, 2020

Apache-HBase commented Jun 2, 2020

huaxiangsun commented Jun 2, 2020

saintstack left a comment

Choose a reason for hiding this comment

saintstack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndimiduk commented Jun 2, 2020

huaxiangsun commented May 27, 2020 •

edited

Loading