HBASE-29933: update_all_config hangs indefinitely when balancing even…#7932
HBASE-29933: update_all_config hangs indefinitely when balancing even…#7932hingu-8103 wants to merge 3 commits intoapache:branch-2from
Conversation
| */ | ||
| @Override | ||
| public void onBalancingStart() { | ||
| LOG.debug("setting isBalancing to true as balance is starting"); |
There was a problem hiding this comment.
nit: Please keep first character of sentence capital.
| */ | ||
| @Override | ||
| public void onBalancingComplete() { | ||
| LOG.debug("setting isBalancing to false as balance is completed"); |
There was a problem hiding this comment.
nit: Please keep first character of sentence capital.
| throw new RuntimeException(e); | ||
| synchronized (this) { | ||
| try { | ||
| wait(sleepTime); |
There was a problem hiding this comment.
Comment line here to explain its giving up monitor.
| for (Map<ServerName, List<RegionInfo>> serverMap : assignments.values()) { | ||
| serverMap.keySet().removeAll(this.serverManager.getDrainingServersList()); | ||
| } | ||
| Map<TableName, Map<ServerName, List<RegionInfo>>> assignments = |
There was a problem hiding this comment.
I can see space character here in diff? Did you fix formatting here and next few lines?
There was a problem hiding this comment.
I guess this is because of introduction of try block. I ran "mvn spotless:apply" to fix formatting.
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
The UT failures look unrelated, but I think we need to review some "synchronized" blocks within BaseLoadBalancer/CacheAwareLoadBalancer.
|
|
||
| @Override | ||
| public synchronized void onConfigurationChange(Configuration conf) { | ||
| public void onConfigurationChange(Configuration conf) { |
There was a problem hiding this comment.
Removing "synchronized" from here will make other Balancer implementations (StochasticLoadBalancer, SimpleBalancer) non thread-safe. And it looks that CacheAwareLoadBalancer.loadConf logic is inside a synchronized block altogether, so better to keep the synchronized here, rather than defining a syncrhonized block in CacheAwareLoadBalancer.loadConf.
There was a problem hiding this comment.
@wchevreuil My bad, I missed that part. I've done the changes and executed the test related to CacheAwareLoadBalancer as well.
Thanks.
3 participants
…t is in progress.
This solves the problem of update config client call hangs indefinitely when balancing event is in progress.
If update config call comes during balance run then it will store the configuration as pending configuration and apply it post balance run.
I've manually tested the change and also added one integration test that verifies that