HBASE-29735 Fix miscalculated timeoutInSeconds when using MoveWithAck

[jcongithub]

The PR tries to fix an issue in RegionMover when ack mode is enabled. It is related to HBASE-29734

Issues
In RegionMover, timeoutInSeconds was calculated without considering retry attempts MoveWithAck. So waitMoveTasksToFinish may exit even though there are remaining retries in MoveWithAck.

Solution
In RegionMover, use regions * wait_per_region * retries to calculate timeoutInSeconds

[PDavid]

Hi @jcongithub,

Thanks for your contribution.
Can you please add the Jira ticket ID to the commit message - e.g. use the commit message format documented here:
https://hbase.apache.org/book.html#hbase.commit.msg.format

Please also update the PR title to start with the Jira ticket ID.

[jcongithub]

Thanks @PDavid

Updated

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 32s		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 _
+1 💚	mvninstall	4m 27s		master passed
+1 💚	compile	3m 46s		master passed
+1 💚	checkstyle	1m 11s		master passed
+1 💚	spotbugs	1m 52s		master passed
+1 💚	spotless	1m 0s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 44s		the patch passed
+1 💚	compile	3m 59s		the patch passed
+1 💚	javac	3m 59s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 16s		the patch passed
+1 💚	spotbugs	2m 14s		the patch passed
+1 💚	hadoopcheck	14m 36s		Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
+1 💚	spotless	0m 55s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 12s		The patch does not generate ASF License warnings.
		48m 23s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7519/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#7519
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 9a19caf5c512 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 / ae470c1
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	84 (vs. ulimit of 30000)
modules	C: hbase-server U: hbase-server
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7519/4/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]

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 41s		Docker mode activated.
-0 ⚠️	yetus	0m 4s		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 _
+1 💚	mvninstall	5m 0s		master passed
+1 💚	compile	1m 10s		master passed
+1 💚	javadoc	0m 41s		master passed
+1 💚	shadedjars	6m 59s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	4m 17s		the patch passed
+1 💚	compile	1m 12s		the patch passed
+1 💚	javac	1m 12s		the patch passed
+1 💚	javadoc	0m 43s		the patch passed
+1 💚	shadedjars	6m 59s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	219m 36s		hbase-server in the patch passed.
		251m 31s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7519/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#7519
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux c2cfabd5acee 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 / ae470c1
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7519/4/testReport/
Max. process+thread count	4638 (vs. ulimit of 30000)
modules	C: hbase-server U: hbase-server
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7519/4/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.

[guluo2016]

It seems better to use this.conf instead of admin.getConfiguration(), conf is an inherited field

[jcongithub]

Thanks @guluo2016 for reviewing the PR.

While this.conf would also work, I chose to continue using admin.getConfiguration() for consistency and correctness:

The admin object is constructed from this.conf (see RegionMover lines 130–131), so admin.getConfiguration() should reflect the same configuration.

The original code already uses admin.getConfiguration() here, and several other places in this class also rely on the admin instance for configuration.

MoveWithAck follows the same pattern and uses admin to retrieve the configuration.

To keep the code consistent across the class and related logic, I think it’s cleaner to continue using admin.getConfiguration().

Please let me what you think. Thanks

[guluo2016]

Is it possible to add a unit test for this?

[jcongithub]

@guluo2016
It seems difficult to add a clean unit test for this scenario. I explored options to simulate a slow region move using SingleProcessHBaseCluster, but there doesn’t appear to be a straightforward way to “hook” into the cluster and delay the region movement without making significant structural changes.

I also looked into mocking MoveWithAck to force a delay/timeout, but I haven’t found a reliable approach there either.

If you know of an existing mechanism or have suggestions on how to simulate a slow region move in the current test framework, I’m happy to add a unit test. Otherwise, adding one may require non-trivial changes to the testing infrastructure.