bharathv commented on a change in pull request #2523:
URL: https://github.com/apache/hbase/pull/2523#discussion_r509861954
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -71,38 +76,78 @@
static final String MERGE_MIN_REGION_SIZE_MB_KEY =
"hbase.normalizer.merge.min_region_size.mb";
static final int DEFAULT_MERGE_MIN_REGION_SIZE_MB = 1;
- private Configuration conf;
private MasterServices masterServices;
- private boolean splitEnabled;
- private boolean mergeEnabled;
- private int minRegionCount;
- private Period mergeMinRegionAge;
- private int mergeMinRegionSizeMb;
+
+ /** Ensure configuration changes are applied atomically. */
+ private final ReadWriteLock configUpdateLock = new ReentrantReadWriteLock();
+ @GuardedBy("configUpdateLock") private Configuration conf;
+ @GuardedBy("configUpdateLock") private boolean splitEnabled;
+ @GuardedBy("configUpdateLock") private boolean mergeEnabled;
+ @GuardedBy("configUpdateLock") private int minRegionCount;
+ @GuardedBy("configUpdateLock") private Period mergeMinRegionAge;
+ @GuardedBy("configUpdateLock") private int mergeMinRegionSizeMb;
public SimpleRegionNormalizer() {
- splitEnabled = DEFAULT_SPLIT_ENABLED;
- mergeEnabled = DEFAULT_MERGE_ENABLED;
- minRegionCount = DEFAULT_MIN_REGION_COUNT;
- mergeMinRegionAge = Period.ofDays(DEFAULT_MERGE_MIN_REGION_AGE_DAYS);
- mergeMinRegionSizeMb = DEFAULT_MERGE_MIN_REGION_SIZE_MB;
+ final Lock writeLock = configUpdateLock.writeLock();
+ writeLock.lock();
+ try {
+ splitEnabled = DEFAULT_SPLIT_ENABLED;
+ mergeEnabled = DEFAULT_MERGE_ENABLED;
+ minRegionCount = DEFAULT_MIN_REGION_COUNT;
+ mergeMinRegionAge = Period.ofDays(DEFAULT_MERGE_MIN_REGION_AGE_DAYS);
+ mergeMinRegionSizeMb = DEFAULT_MERGE_MIN_REGION_SIZE_MB;
+ } finally {
+ writeLock.unlock();
+ }
}
@Override
public Configuration getConf() {
- return conf;
+ final Lock readLock = configUpdateLock.readLock();
+ readLock.lock();
+ try {
+ return conf;
+ } finally {
+ readLock.unlock();
+ }
}
@Override
public void setConf(final Configuration conf) {
if (conf == null) {
return;
}
- this.conf = conf;
- splitEnabled = conf.getBoolean(SPLIT_ENABLED_KEY, DEFAULT_SPLIT_ENABLED);
- mergeEnabled = conf.getBoolean(MERGE_ENABLED_KEY, DEFAULT_MERGE_ENABLED);
- minRegionCount = parseMinRegionCount(conf);
- mergeMinRegionAge = parseMergeMinRegionAge(conf);
- mergeMinRegionSizeMb = parseMergeMinRegionSizeMb(conf);
+
+ final Lock writeLock = configUpdateLock.writeLock();
+ writeLock.lock();
Review comment:
Stumbled upon this change. I think there is a much simpler way to
achieve this without locks and fewer lines of code (and cleaner). If we can
factor all of the configs into a single object (with appropriate getters and
setters if needed), something like,
```
static class NormalizerConfig {
Configuration conf;
boolean splitEnabled;
private boolean mergeEnabled;
private Period mergeMinRegionAge;
private int mergeMinRegionSizeMb;
.......
static parseFromConfig(Conf conf);
}
private NormalizerConfig normalizerConf;
public void setConf(final Configuration conf) {
normalizerConf = parseFromConfig(conf);
}
public boolean isSplitEnabled() {
return normalizerConf.isSplitEnabled();
}
```
Reference assignment is atomic. So even if multiple threads call
setConf(conf), each thread calls its own parseFromConfig() in it's own context,
constructs the whole object and the reference assignment works cleanly. On the
reader side depending on what reference is being used that point, the value is
returned (ex: isSplitEnabled() above)..
The advantage of using these locks is the memory ordering that they enforce
in methods like isSplitEnabled(). We essentially block until the reference is
updated but I don't think that is a requirement here because we don't guarantee
the callers of these methods (like computePlansForTable()) that they will work
on the latest config while the config update is in progress (we can't guarantee
that level of ordering anyway). Point here being the above approach gets rid of
most code and is still not racy. WDYT.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
