vaijosh commented on code in PR #7900:
URL: https://github.com/apache/hbase/pull/7900#discussion_r2922245063
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java:
##########
@@ -396,7 +408,26 @@ public void regionOffline(RegionInfo regionInfo) {
}
@Override
- public synchronized void onConfigurationChange(Configuration conf) {
+ public void onConfigurationChange(Configuration conf) {
+ // Refer to HBASE-29933
+ synchronized (this) {
+ // If balance is running, store configuration in pendingConfiguration
and return immediately.
+ // Deferred config update.
Review Comment:
nit: I'd write it as
// If balance is running, store configuration in pendingConfiguration and
return immediately. Defer the config update.
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java:
##########
@@ -171,4 +171,17 @@ Map<ServerName, List<RegionInfo>>
retainAssignment(Map<RegionInfo, ServerName> r
default void throttle(RegionPlan plan) throws Exception {
// noop
}
+
+ default long getThrottlingTime(RegionPlan plan) {
Review Comment:
@hingu-8103
IMO getThrottlingTime is ambiguous ( is it timestamp or a duration). I’d
prefer something like getThrottleDurationMs (or getThrottleDelayMs). This
method name will make it clear that its throttle duration in milliseconds.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java:
##########
@@ -396,7 +408,26 @@ public void regionOffline(RegionInfo regionInfo) {
}
@Override
- public synchronized void onConfigurationChange(Configuration conf) {
+ public void onConfigurationChange(Configuration conf) {
+ // Refer to HBASE-29933
+ synchronized (this) {
+ // If balance is running, store configuration in pendingConfiguration
and return immediately.
+ // Deferred config update.
+ if (isBalancing.get()) {
+ LOG.debug(
+ "Balance is in progress, defer applying configuration change until
balance completed.");
+ pendingConfiguration.set(conf);
+ } else {
+ // If balance is not running, apply configuration change immediately.
Review Comment:
We are already in else block means balancer not running, so IMO no need to
write "If balance is not running" in the comment. Please keep it as
//apply configuration change immediately
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java:
##########
@@ -407,6 +438,51 @@ public synchronized void
onConfigurationChange(Configuration conf) {
internalBalancer.onConfigurationChange(conf);
}
+ /**
+ * If a pending configuration was stored during a balance run, applies it
now by calling
Review Comment:
nit: I'd write it as
"If a pending configuration was stored during a balance run, apply it and
clear the pending reference"
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java:
##########
@@ -396,7 +408,26 @@ public void regionOffline(RegionInfo regionInfo) {
}
@Override
- public synchronized void onConfigurationChange(Configuration conf) {
+ public void onConfigurationChange(Configuration conf) {
+ // Refer to HBASE-29933
+ synchronized (this) {
+ // If balance is running, store configuration in pendingConfiguration
and return immediately.
+ // Deferred config update.
+ if (isBalancing.get()) {
+ LOG.debug(
+ "Balance is in progress, defer applying configuration change until
balance completed.");
+ pendingConfiguration.set(conf);
+ } else {
+ // If balance is not running, apply configuration change immediately.
+ updateConfiguration(conf);
+ }
+ }
+ }
+
+ /**
+ * Applies the given configuration immediately.
Review Comment:
nit: Please remove word "immediately".
--
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]
