Apache9 commented on a change in pull request #3660:
URL: https://github.com/apache/hbase/pull/3660#discussion_r717572621
##########
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##########
@@ -295,6 +295,7 @@ message IsInMaintenanceModeResponse {
message BalanceRequest {
optional bool ignore_rit = 1;
optional bool dry_run = 2;
+ optional bool reload_configs = 4;
Review comment:
Why 4?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/MaintenanceLoadBalancer.java
##########
@@ -121,4 +121,9 @@ public void postMasterStartupInitialize() {
@Override
public void updateBalancerStatus(boolean status) {
}
+
+ @Override
+ public void reloadConfiguration() {
+
Review comment:
nits: remove the empty line.
##########
File path:
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
##########
@@ -617,4 +617,14 @@ private void roundRobinAssignment(BalancerClusterState
cluster, List<RegionInfo>
public void onConfigurationChange(Configuration conf) {
loadConf(conf);
}
+
+ @Override
+ public void reloadConfiguration() {
+ LOG.info("Reloading balancer configs");
+ // We clone and reload the clone because the original conf may be used in
other
+ // non-balancer contexts. We only want to reload configs for the balancer.
+ Configuration conf = new Configuration(getConf());
Review comment:
This is a bit confusing to developers I think. Here we will create a new
Configuration object, but for other places, we just the Configuration instance
of master?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
##########
@@ -1192,6 +1192,10 @@ public BalanceResponse balanceRSGroup(String groupName,
BalanceRequest request)
return responseBuilder.build();
}
+ if (request.isReloadConfigs()) {
Review comment:
I think we'd better put the clone and reload logic here, and then just
call balancer.onConfigurationChange method? This will make the patch smaller,
and we do not need to modify the load configuration related method in balancer.
##########
File path:
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
##########
@@ -617,4 +617,14 @@ private void roundRobinAssignment(BalancerClusterState
cluster, List<RegionInfo>
public void onConfigurationChange(Configuration conf) {
loadConf(conf);
}
+
+ @Override
+ public void reloadConfiguration() {
+ LOG.info("Reloading balancer configs");
+ // We clone and reload the clone because the original conf may be used in
other
+ // non-balancer contexts. We only want to reload configs for the balancer.
+ Configuration conf = new Configuration(getConf());
Review comment:
I think it is fine to just reload the balancer's config, the concern is
about the place.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
##########
@@ -1192,6 +1192,10 @@ public BalanceResponse balanceRSGroup(String groupName,
BalanceRequest request)
return responseBuilder.build();
}
+ if (request.isReloadConfigs()) {
Review comment:
Introduce a util method called reloadBalancerConfig and add some
comments to say it? Just want to make the balancer code itself clean.
--
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]
