Skip to content

Conversation

@ajkh88
Copy link
Contributor

@ajkh88 ajkh88 commented Dec 17, 2025

This change adds fail-fast validation that throws UnknownRegionException when callers attempt to reopen regions that don't belong to the specified table, preventing silent failures and configuration errors in production. The implementation removes unused batch parameters from the API in favor of configuration-based throttling (via table descriptor or global config), ensuring consistent behavior between reopening all regions and specific subsets. Exception handling now properly fails procedures for DoNotRetry errors rather than infinitely retrying validation failures.

JIRA: https://issues.apache.org/jira/browse/HBASE-29782

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 35s 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.
+0 🆗 buf 0m 0s buf was not available.
+0 🆗 buf 0m 0s buf 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 18s Maven dependency ordering for branch
+1 💚 mvninstall 4m 4s master passed
+1 💚 compile 7m 10s master passed
+1 💚 checkstyle 3m 15s master passed
+1 💚 spotbugs 7m 56s master passed
+1 💚 spotless 0m 59s branch has no errors when running spotless:check.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 12s Maven dependency ordering for patch
+1 💚 mvninstall 3m 36s the patch passed
+1 💚 compile 6m 48s the patch passed
+1 💚 cc 6m 48s the patch passed
+1 💚 javac 6m 48s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 2m 46s the patch passed
+1 💚 spotbugs 8m 8s the patch passed
+1 💚 hadoopcheck 12m 59s Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
+1 💚 hbaseprotoc 2m 14s the patch passed
+1 💚 spotless 0m 56s patch has no errors when running spotless:check.
_ Other Tests _
+1 💚 asflicense 0m 43s The patch does not generate ASF License warnings.
71m 43s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7563/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #7563
Optional Tests dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless cc buflint bufcompat hbaseprotoc
uname Linux cb83557df01f 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 / 57dc513
Default Java Eclipse Adoptium-17.0.11+9
Max. process+thread count 85 (vs. ulimit of 30000)
modules C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7563/2/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
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 31s 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 17s Maven dependency ordering for branch
+1 💚 mvninstall 3m 35s master passed
+1 💚 compile 2m 21s master passed
+1 💚 javadoc 1m 27s master passed
+1 💚 shadedjars 6m 21s branch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 15s Maven dependency ordering for patch
+1 💚 mvninstall 3m 11s the patch passed
+1 💚 compile 2m 22s the patch passed
+1 💚 javac 2m 22s the patch passed
-0 ⚠️ javadoc 0m 27s /results-javadoc-javadoc-hbase-server.txt hbase-server generated 2 new + 63 unchanged - 0 fixed = 65 total (was 63)
+1 💚 shadedjars 6m 14s patch has no errors when building our shaded downstream artifacts.
_ Other Tests _
+1 💚 unit 0m 35s hbase-protocol-shaded in the patch passed.
+1 💚 unit 1m 34s hbase-client in the patch passed.
+1 💚 unit 212m 57s hbase-server in the patch passed.
+1 💚 unit 6m 45s hbase-thrift in the patch passed.
255m 8s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7563/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #7563
Optional Tests javac javadoc unit compile shadedjars
uname Linux 0608b7dc643f 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 / 57dc513
Default Java Eclipse Adoptium-17.0.11+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7563/2/testReport/
Max. process+thread count 4254 (vs. ulimit of 30000)
modules C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7563/2/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.

}

ReopenTableRegionsProcedure(final TableName tableName, long reopenBatchBackoffMillis,
public ReopenTableRegionsProcedure(final TableName tableName, long reopenBatchBackoffMillis,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you made these public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use in an integration test - do I need to annotate the methods in some way to indicate they are only exposed for testing? Something akin to @VisibleForTesting from Guava perhaps?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's going to be a subsequent PR that introduces the integration test, please wait and make the change then.

Yes, my recollection is that there is a VisibleForTesting on the classpath. I don't think we use its presence for any static enforcement, but it is useful enough to communicate intent. IMHO a comment is sufficient.

* @return procedure Id
* @throws IOException if reopening region fails while running procedure
*/
long reopenRegionsThrottled(final TableName tableName, final List<byte[]> regionNames,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throttling is a net new feature with this change? I'm surprised it's not introduced as a separate feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throttling has already been implemented in the ReopenTableRegionsProcedure, but for some reason the existing method on HMaster doesn't use it. I didn't want to change the existing implementation so I exposed a new method to allow users to choose - and used it for these changes too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file a ticket to report the other flow not making use of this method? We should centralize the implementations if we can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants