-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29782 Expose public Admin API to reopen table regions without moving #7563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| } | ||
|
|
||
| ReopenTableRegionsProcedure(final TableName tableName, long reopenBatchBackoffMillis, | ||
| public ReopenTableRegionsProcedure(final TableName tableName, long reopenBatchBackoffMillis, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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