ankitsinghal commented on a change in pull request #4181:
URL: https://github.com/apache/hbase/pull/4181#discussion_r823204995
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -389,7 +391,7 @@
// the key is table name, the value is the number of compactions in that
table.
private Map<TableName, AtomicInteger> mobCompactionStates =
Maps.newConcurrentMap();
- MasterCoprocessorHost cpHost;
+ volatile MasterCoprocessorHost cpHost;
Review comment:
Just a casual comment here:- Volatile comes with a performance penalty
because of the constraints the JVM optimizer has to follow and extra memory
copy. But at the same time, I don't know if we can measure the impact yet.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -4088,6 +4090,12 @@ public void onConfigurationChange(Configuration newConf)
{
} catch (IOException e) {
LOG.warn("Failed to initialize SuperUsers on reloading of the
configuration");
}
+ // update region server coprocessor if the configuration has changed.
+ if
(CoprocessorConfigurationUtil.checkConfigurationChange(getConfiguration(),
newConf,
+ CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY)) {
+ LOG.info("Update the master coprocessor(s) because the configuration has
changed");
+ this.cpHost = new MasterCoprocessorHost(this, newConf);
+ }
Review comment:
Don't we need to handle maintenance mode here?
And how are we ensuring that newConf includes the System coprocessor we are
adding based on whether the feature is on or not?
For eg: QuotasObserver
conf.setStrings(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY,
MasterQuotasObserver.class.getName());
Another is RSGroupAdminEndpoint?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -8604,6 +8605,13 @@ IOException throwOnInterrupt(Throwable t) {
@Override
public void onConfigurationChange(Configuration conf) {
this.storeHotnessProtector.update(conf);
+ // update coprocessorHost if the configuration has changed.
+ if
(CoprocessorConfigurationUtil.checkConfigurationChange(getReadOnlyConfiguration(),
conf,
+ CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
+ CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY)) {
+ LOG.info("Update the system coprocessors because the configuration has
changed");
+ this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf);
Review comment:
HRegion.decorateRegionConfiguration(conf) ?
--
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]
