wchevreuil commented on code in PR #7900:
URL: https://github.com/apache/hbase/pull/7900#discussion_r2918912988
##########
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:
You moved almost all synchronisation logic from CacheAwareLoadBalancer to
RSGroupBasedLoadBalancer, so why not simply remove the synchronized from
CacheAwareLoadBalancer.throttle, then just call internalBalancer.throttle
here? IIUC, this throttle method is already called from a synchronized context
initiated in HMaster.balance. That would be more cohesive, as now the entire
syncrhonization is on the same place and no need to explicit refer to
CacheAwareLoadBalancer from here.
--
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]
