virajjasani commented on a change in pull request #2523:
URL: https://github.com/apache/hbase/pull/2523#discussion_r509121515
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -71,38 +76,81 @@
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 readLock = configUpdateLock.readLock();
+ final Lock writeLock = configUpdateLock.writeLock();
+ writeLock.lock(); // "a writer can acquire the read lock, but not
vice-versa."
+ readLock.lock();
Review comment:
I thought of the same initially but then realized that we don't want
half updated configs when we read them with `getConf()` and make half correct
decisions. Plus, since this is operator triggered action and not self triggered
action, it's once in a while (Just in case you are more worried about threads
reading `conf` getting blocked on readLock).
Still, let's wait for @ndimiduk 's response in case i might have missed some
improvement here.
----------------------------------------------------------------
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]
