hingu-8103 commented on code in PR #7900:
URL: https://github.com/apache/hbase/pull/7900#discussion_r2919025135
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java:
##########
@@ -407,6 +438,56 @@ public synchronized void
onConfigurationChange(Configuration conf) {
internalBalancer.onConfigurationChange(conf);
}
+ /**
+ * If a pending configuration was stored during a balance run, applies it
now by calling
+ * {@link #updateConfiguration(Configuration)} and clears the pending
reference.
+ */
+ public void applyPendingConfiguration() {
+ Configuration toApply = pendingConfiguration.getAndSet(null);
+ if (toApply != null) {
+ LOG.info("Applying pending configuration after balance completed.");
+ updateConfiguration(toApply);
+ }
+ }
+
+ /**
+ * Sets {@link #isBalancing} to {@code true} before a balance run starts.
+ */
+ @Override
+ public void onBalancingStart() {
+ LOG.debug("setting isBalancing to true as balance is starting");
+ isBalancing.set(true);
+ }
+
+ /**
+ * Sets {@link #isBalancing} to {@code false} after a balance run completes
and applies any
+ * pending configuration that arrived during balancing.
+ */
+ @Override
+ public void onBalancingComplete() {
+ LOG.debug("setting isBalancing to false as balance is completed");
+ isBalancing.set(false);
+ applyPendingConfiguration();
+ }
+
+ @Override
+ public void throttle(RegionPlan plan) throws Exception {
+ // For CacheAwareLoadBalancer, get the throttle delay and perform wait()
here
+ // to keep synchronization logic in one place
+ if (internalBalancer instanceof CacheAwareLoadBalancer) {
+ long throttleTime = ((CacheAwareLoadBalancer)
internalBalancer).getThrottleTime(plan);
+ if (throttleTime > 0) {
+ try {
+ wait(throttleTime);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ } else {
+ internalBalancer.throttle(plan);
+ }
Review Comment:
Hi @wchevreuil , I've refactored the code and now throttling logic is in
RSGroupBasedLoadBalancer.throttle() so wait() call can release the same monitor
held by HMaster.balance(). I also refactored the tests of
CacheAwareLoadBalancer to use CacheAwareLoadBalancer as internalBalancer in
RSGroupBasedLoadBalancer as that is the expected behaviour by default in master
branch.
--
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]
