jcongithub commented on code in PR #7519:
URL: https://github.com/apache/hbase/pull/7519#discussion_r2638095313
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java:
##########
@@ -396,8 +399,13 @@ private void loadRegions(List<RegionInfo> regionsToMove)
throws Exception {
}
moveRegionsPool.shutdown();
+ // Calculate timeout based on acknowledge mode. In ack mode, account for
retry attempts since
+ // MoveWithAck retries failed moves up to MOVE_RETRIES_MAX_KEY times. In
no-ack mode, each
+ // region move is attempted only once. Timeout = regions * wait_per_region
* retries
+ int retries = (ack) ? maxRetries : 1;
long timeoutInSeconds = regionsToMove.size()
- * admin.getConfiguration().getLong(MOVE_WAIT_MAX_KEY,
DEFAULT_MOVE_WAIT_MAX);
+ * admin.getConfiguration().getLong(MOVE_WAIT_MAX_KEY,
DEFAULT_MOVE_WAIT_MAX) * retries;
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
