virajjasani commented on a change in pull request #2523:
URL: https://github.com/apache/hbase/pull/2523#discussion_r508380812
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -140,39 +188,76 @@ private static int parseMergeMinRegionSizeMb(final
Configuration conf) {
key, parsedValue, settledValue);
}
+ private static <T> void logConfigurationUpdated(final String key, final T
oldValue,
+ final T newValue) {
+ if (!Objects.equals(oldValue, newValue)) {
+ LOG.info("Updated configuration for key '{}' from {} to {}", key,
oldValue, newValue);
+ }
+ }
+
/**
* Return this instance's configured value for {@value #SPLIT_ENABLED_KEY}.
*/
public boolean isSplitEnabled() {
- return splitEnabled;
+ final Lock readLock = configUpdateLock.readLock();
+ readLock.lock();
+ try {
+ return splitEnabled;
+ } finally {
+ readLock.unlock();
+ }
}
/**
* Return this instance's configured value for {@value #MERGE_ENABLED_KEY}.
*/
public boolean isMergeEnabled() {
- return mergeEnabled;
+ final Lock readLock = configUpdateLock.readLock();
+ readLock.lock();
+ try {
+ return mergeEnabled;
+ } finally {
+ readLock.unlock();
+ }
}
/**
* Return this instance's configured value for {@value
#MIN_REGION_COUNT_KEY}.
*/
public int getMinRegionCount() {
- return minRegionCount;
+ final Lock readLock = configUpdateLock.readLock();
+ readLock.lock();
+ try {
+ return minRegionCount;
+ } finally {
+ readLock.unlock();
+ }
}
/**
* Return this instance's configured value for {@value
#MERGE_MIN_REGION_AGE_DAYS_KEY}.
*/
public Period getMergeMinRegionAge() {
- return mergeMinRegionAge;
+ final Lock readLock = configUpdateLock.readLock();
+ readLock.lock();
+ try {
+ return mergeMinRegionAge;
+ } finally {
+ readLock.unlock();
+ }
}
/**
* Return this instance's configured value for {@value
#MERGE_MIN_REGION_SIZE_MB_KEY}.
*/
public int getMergeMinRegionSizeMb() {
Review comment:
Last 3 getters should have `@VisibleForTesting` ?
##########
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;
Review comment:
Indeed ! Wondering if we can ensure all implementors of
`ConfigurationObserver` can start using such lock for atomic updates of
non-final fields (of course not as part of this Jira :) )
----------------------------------------------------------------
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:
us...@infra.apache.org
