HBASE-29168 Add configurable throttling of region moves in CacheAwareLoadBalancer. #6763

wchevreuil · 2025-03-06T14:31:18Z

No description provided.

…LoadBalancer. Change-Id: I5af00b9e661c5e0a38ab5c346bd49349d8d01a74

Change-Id: I4039f703c34e12cc43d5e728e65da8de0a89ab2a

Change-Id: I5353a787667dc7a3d38d1a9a2b60ce5ef58bc4a6

rmdmattingly · 2025-03-10T10:52:06Z

...e-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java

 public class CacheAwareLoadBalancer extends StochasticLoadBalancer {
  private static final Logger LOG = LoggerFactory.getLogger(CacheAwareLoadBalancer.class);

+  public static final String CACHE_RATIO_THRESHOLD = "hbase.master.balancer.stochastic.cacheRatio";


If this ratio is only used for throttling, then I wonder whether we should name it more specifically

rmdmattingly · 2025-03-10T10:52:44Z

...e-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java


+  public static final String CACHE_RATIO_THRESHOLD = "hbase.master.balancer.stochastic.cacheRatio";
+
+  public static final String MOVE_THROTTLING = "hbase.master.balancer.move.throttlingMs";


If throttle support is only added to the cache aware balancer, then maybe this configuration name should also be specific to the cache aware balancer

Good point. Right now, it's only implemented by the cache aware balancer. But since it may be implemented by other possible balancer implementations in the future, I thought may be worth move these two to the LoadBalancer interface.

rmdmattingly · 2025-03-10T10:54:19Z

...e-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java

+
+  public static final String MOVE_THROTTLING = "hbase.master.balancer.move.throttlingMs";
+
+  public static final long MOVE_THROTTLING_DEFAULT = 60 * 1000;


nit: the unit is ambiguous. We should either use a more intentional type like Duration, or we should include the unit in the name

Decided to go with using the Duration suggestion. This is now moved to the LoadBalancer interface (see my reply in other comment above).

rmdmattingly · 2025-03-10T10:55:59Z

...e-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java

+    float ratioThreshold =
+      this.configuration.getFloat(CACHE_RATIO_THRESHOLD, CACHE_RATIO_THRESHOLD_DEFAULT);


This could be a pretty hot path on a bigger, imbalanced cluster. So I wonder if we should move this conf lookup to somewhere less hot?

Moved this and the sleepTime to the loadConf() method

Change-Id: Ie2882a3714775cd883ac2895785c3ace0977f3a9

wchevreuil · 2025-03-10T22:25:09Z

Thanks for your suggestions, @rmdmattingly ! I had applied them, please review it again.

wchevreuil · 2025-03-12T10:58:28Z

Latest UT failure isn't related. I believe it's a flakey and had submitted a separate fix for that at HBASE-29183.

wchevreuil · 2025-03-16T13:46:33Z

re-test

Apache-HBase · 2025-03-19T15:16:00Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 36s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ master Compile Tests _
+0 🆗	mvndep	0m 14s		Maven dependency ordering for branch
+1 💚	mvninstall	3m 14s		master passed
+1 💚	compile	3m 29s		master passed
+1 💚	checkstyle	0m 47s		master passed
+1 💚	spotbugs	1m 58s		master passed
+1 💚	spotless	0m 46s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 0s		the patch passed
+1 💚	compile	3m 29s		the patch passed
-0 ⚠️	javac	0m 24s	/results-compile-javac-hbase-balancer.txt	hbase-balancer generated 1 new + 60 unchanged - 0 fixed = 61 total (was 60)
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 9s	/results-checkstyle-hbase-balancer.txt	hbase-balancer: The patch generated 1 new + 2 unchanged - 0 fixed = 3 total (was 2)
+1 💚	spotbugs	2m 10s		the patch passed
+1 💚	hadoopcheck	11m 56s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 45s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 18s		The patch does not generate ASF License warnings.
		41m 19s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6763/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6763
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 15a7176788ec 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `b2af54e`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	86 (vs. ulimit of 30000)
modules	C: hbase-balancer hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6763/5/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2025-03-19T19:00:14Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 29s		Docker mode activated.
-0 ⚠️	yetus	0m 3s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ master Compile Tests _
+0 🆗	mvndep	0m 11s		Maven dependency ordering for branch
+1 💚	mvninstall	3m 16s		master passed
+1 💚	compile	1m 13s		master passed
+1 💚	javadoc	0m 40s		master passed
+1 💚	shadedjars	5m 59s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 5s		the patch passed
+1 💚	compile	1m 14s		the patch passed
+1 💚	javac	1m 14s		the patch passed
+1 💚	javadoc	0m 38s		the patch passed
+1 💚	shadedjars	5m 58s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	7m 32s		hbase-balancer in the patch passed.
+1 💚	unit	213m 35s		hbase-server in the patch passed.
		248m 40s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6763/5/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6763
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 4ae4a7946890 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `b2af54e`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6763/5/testReport/
Max. process+thread count	5274 (vs. ulimit of 30000)
modules	C: hbase-balancer hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6763/5/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

taklwu

LGTM

taklwu · 2025-03-21T21:00:56Z

...server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockEvictionOnRegionMovement.java

    conf.set("hbase.bucketcache.persistent.path", testDir + "/bucket.persistence");
    conf.setLong(CacheConfig.BUCKETCACHE_PERSIST_INTERVAL_KEY, 100);
    conf.setBoolean(CacheConfig.EVICT_BLOCKS_ON_CLOSE_KEY, true);
+    conf.setBoolean(CACHE_BLOCKS_ON_WRITE_KEY, true);


[nit] why do we need to add the cache blocks on write here? does anyone change the test behavior? I cannot find any other edits in this file.

Yes, this is not related to this throttling change, but I found out this test was flakey, with the assertion from line 104 failing sometimes. Most of the times, it will pass, as once we flush the newly added data on line 95, we'll eventually open the newly created file and run a prefetch for it. But this happens in the background, and sometimes, the prefetch might not have cached any blocks when we get to line 104 of this test. So, if we want to make sure blocks are cached at that point, we should have blockOnWrite set to true.

taklwu · 2025-03-21T21:04:19Z

...e-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java


+  public static final String CACHE_RATIO_THRESHOLD =
+    "hbase.master.balancer.stochastic.throttling.cacheRatio";
+  public static final float CACHE_RATIO_THRESHOLD_DEFAULT = 0.8f;


[nit] do you think we need a doc update about this feature ?

Good point, I'll add it to the cache aware balancer doc section. Let me do it on a separate jira once this get merged.

…LoadBalancer. (#6763) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>

…LoadBalancer. (apache#6763) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Change-Id: I1c9df00f1723246685771f69930b1e5f89db72ff

…LoadBalancer. (#6763) (#6865) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>

…LoadBalancer. (#6763) (#6865) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Change-Id: Iaac6f08aa0d45c173f80da4ccef7ec90df9552dd

…LoadBalancer. (apache#6763) (apache#6865) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>

HBASE-29168 Add configurable throttling of region moves in CacheAware…

c9b1473

…LoadBalancer. Change-Id: I5af00b9e661c5e0a38ab5c346bd49349d8d01a74

This comment has been minimized.

Sign in to view

attempt to fix unrelated flakey tests

b758bd6

Change-Id: I4039f703c34e12cc43d5e728e65da8de0a89ab2a

wchevreuil requested a review from chrajeshbabu March 7, 2025 11:03

This comment has been minimized.

Sign in to view

addressing spotless failure

4b2c50f

Change-Id: I5353a787667dc7a3d38d1a9a2b60ce5ef58bc4a6

This comment has been minimized.

Sign in to view

rmdmattingly reviewed Mar 10, 2025

View reviewed changes

addressing review comments

b2af54e

Change-Id: Ie2882a3714775cd883ac2895785c3ace0977f3a9

wchevreuil requested a review from rmdmattingly March 10, 2025 22:24

This comment has been minimized.

Sign in to view

taklwu approved these changes Mar 21, 2025

View reviewed changes

wchevreuil merged commit d98c63a into apache:master Mar 27, 2025
1 check passed

wchevreuil added a commit that referenced this pull request Mar 27, 2025

HBASE-29168 Add configurable throttling of region moves in CacheAware…

833500a

…LoadBalancer. (#6763) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>

wchevreuil added a commit that referenced this pull request Mar 31, 2025

HBASE-29168 Add configurable throttling of region moves in CacheAware…

4d9bf56

…LoadBalancer. (#6763) (#6865) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>

mokai87 pushed a commit to mokai87/hbase that referenced this pull request Aug 7, 2025

HBASE-29168 Add configurable throttling of region moves in CacheAware…

bbd6c86

…LoadBalancer. (apache#6763) (apache#6865) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>


		public static final String CACHE_RATIO_THRESHOLD = "hbase.master.balancer.stochastic.cacheRatio";

		public static final String MOVE_THROTTLING = "hbase.master.balancer.move.throttlingMs";


		public static final String MOVE_THROTTLING = "hbase.master.balancer.move.throttlingMs";

		public static final long MOVE_THROTTLING_DEFAULT = 60 * 1000;

		float ratioThreshold =
		this.configuration.getFloat(CACHE_RATIO_THRESHOLD, CACHE_RATIO_THRESHOLD_DEFAULT);

HBASE-29168 Add configurable throttling of region moves in CacheAwareLoadBalancer. #6763

HBASE-29168 Add configurable throttling of region moves in CacheAwareLoadBalancer. #6763

Uh oh!

Conversation

wchevreuil commented Mar 6, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchevreuil commented Mar 10, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

wchevreuil commented Mar 12, 2025

Uh oh!

wchevreuil commented Mar 16, 2025

Uh oh!

Apache-HBase commented Mar 19, 2025

Uh oh!

Apache-HBase commented Mar 19, 2025

Uh oh!

taklwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants