HBASE-25167 Normalizer support for hot config reloading#2523
HBASE-25167 Normalizer support for hot config reloading#2523ndimiduk merged 1 commit intoapache:masterfrom
Conversation
ndimiduk
requested review from
huaxiangsun,
infraio,
joshelser,
saintstack and
virajjasani
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| /** Ensure configuration changes are applied atomically. */ | ||
| private final ReadWriteLock configUpdateLock = new ReentrantReadWriteLock(); | ||
| @GuardedBy("configUpdateLock") private Configuration conf; |
There was a problem hiding this comment.
This is cool. I believe this will help us avoid partial config update!
There was a problem hiding this 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 :) )
There was a problem hiding this comment.
I'm not sure about "all" uses, it just seemed prudent for this use. I'm also not sure our static analysis tools honor this GuardedBy annotation, I have a TODO for myself to track this down and see if it's really supported.
There was a problem hiding this comment.
I have learnt about GuardedBy for the first time on this PR, so I am also not sure if static analysis tools really require some work.
There was a problem hiding this comment.
It seems SpotBugs does have a bug description for the improper use of GuardedBy, https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#is-field-not-guarded-against-concurrent-access-is-field-not-guarded. I wonder if it's actually implemented ;)
|
I think the failure in |
|
We used to have 2 types of hot config reload:
Since I haven't used (looking forward to dig deeper) |
|
@virajjasani to the best of my knowledge, the FYI, the only "configuration" we store in ZK, as far as I know, is for replication. The |
That's true, |
| /** | ||
| * Return this instance's configured value for {@value #MERGE_MIN_REGION_SIZE_MB_KEY}. | ||
| */ | ||
| public int getMergeMinRegionSizeMb() { |
There was a problem hiding this comment.
Last 3 getters should have @VisibleForTesting ?
There was a problem hiding this comment.
In light of HBASE-24640, I didn't want to introduce any new uses of this annotation.
|
|
||
| /** Ensure configuration changes are applied atomically. */ | ||
| private final ReadWriteLock configUpdateLock = new ReentrantReadWriteLock(); | ||
| @GuardedBy("configUpdateLock") private Configuration conf; |
There was a problem hiding this 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 comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| 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(); |
There was a problem hiding this comment.
Why is readLock.lock() called here? Thought writeLock is enough.
There was a problem hiding this 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.
There was a problem hiding this comment.
Why is readLock.lock() called here? Thought writeLock is enough.
Initially I thought so too, but reading the docs on ReadWriteLock, it never explicitly said that the WriteLock is exclusive for both readers and writers, it only says that it's exclusive. The best I can find is from Java Concurrency in Practice, which says (emphasis mine):
... Mutual exclusion is a conservative locking strategy that prevents writer/writer and writer/reader overlap, but also prevents reader/reader overlap. In many cases, data structures are “read-mostly”—they are mutable and are sometimes modified, but most accesses involve only reading. In these cases, it would be nice to relax the locking requirements to allow multiple readers to access the data structure at once. As long as each thread is guaranteed an up-to-date view of the data and no other thread modifies the data while the readers are viewing it, there will be no problems. This is what read-write locks allow: a resource can be accessed by multiple readers or a single writer at a time, but not both.
So I think you're correct @huaxiangsun , the write lock should be sufficient.
There was a problem hiding this comment.
As long as each thread is guaranteed an up-to-date view of the data and no other thread modifies the data while the readers are viewing it, there will be no problems.
Do we not need readLock() + writeLock() to achieve this? How can we achieve writers not updating while readers are reading conf?
There was a problem hiding this comment.
To me, the next sentence, This is what read-write locks allow: a resource can be accessed by multiple readers or a single writer at a time, but not both. To me, that means that by taking a writeLock, the thread holding it has exclusive access. writeLock should be enough.
There was a problem hiding this comment.
But test output suggests otherwise :)
There was a problem hiding this comment.
Or I just did it wrong.
ndimiduk
force-pushed
the
25167-normalizer-config-hot-reload
branch
from
557c6c9 to
ef7269f
Compare
|
Rebase and remove taking the ReadLock from |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
force-pushed
the
25167-normalizer-config-hot-reload
branch
2 times, most recently
from
e1a733f to
8a71147
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| mergeMinRegionSizeMb = parseMergeMinRegionSizeMb(conf); | ||
|
|
||
| final Lock writeLock = configUpdateLock.writeLock(); | ||
| writeLock.lock(); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Sure @bharathv , I think that's a nice suggestion. I agree that the strict ordering guarantees provided by explicit locking are not needed here. I've pushed a new commit that unwinds the locking and uses atomic instance assignment as you suggest. Let me know what you think.
There was a problem hiding this comment.
This is cool, much simpler!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
force-pushed
the
25167-normalizer-config-hot-reload
branch
from
8a71147 to
c8ecefb
Compare
|
🎊 +1 overall
This message was automatically generated. |
Wire up the `ConfigurationObserver` chain for `RegionNormalizerManager`. The following configuration keys support hot-reloading: * hbase.normalizer.throughput.max_bytes_per_sec * hbase.normalizer.split.enabled * hbase.normalizer.merge.enabled * hbase.normalizer.min.region.count * hbase.normalizer.merge.min_region_age.days * hbase.normalizer.merge.min_region_size.mb Note that support for `hbase.normalizer.period` is not provided here. Support would need to be implemented generally for the `Chore` subsystem.
ndimiduk
force-pushed
the
25167-normalizer-config-hot-reload
branch
from
c8ecefb to
fe89fb0
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Wire up the `ConfigurationObserver` chain for `RegionNormalizerManager`. The following configuration keys support hot-reloading: * hbase.normalizer.throughput.max_bytes_per_sec * hbase.normalizer.split.enabled * hbase.normalizer.merge.enabled * hbase.normalizer.min.region.count * hbase.normalizer.merge.min_region_age.days * hbase.normalizer.merge.min_region_size.mb Note that support for `hbase.normalizer.period` is not provided here. Support would need to be implemented generally for the `Chore` subsystem. Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com>
Wire up the `ConfigurationObserver` chain for `RegionNormalizerManager`. The following configuration keys support hot-reloading: * hbase.normalizer.throughput.max_bytes_per_sec * hbase.normalizer.split.enabled * hbase.normalizer.merge.enabled * hbase.normalizer.min.region.count * hbase.normalizer.merge.min_region_age.days * hbase.normalizer.merge.min_region_size.mb Note that support for `hbase.normalizer.period` is not provided here. Support would need to be implemented generally for the `Chore` subsystem. Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com>
Wire up the `ConfigurationObserver` chain for `RegionNormalizerManager`. The following configuration keys support hot-reloading: * hbase.normalizer.throughput.max_bytes_per_sec * hbase.normalizer.split.enabled * hbase.normalizer.merge.enabled * hbase.normalizer.min.region.count * hbase.normalizer.merge.min_region_age.days * hbase.normalizer.merge.min_region_size.mb Note that support for `hbase.normalizer.period` is not provided here. Support would need to be implemented generally for the `Chore` subsystem. Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com>
6 participants
Wire up the
ConfigurationObserverchain forRegionNormalizerManager. The following configuration keys support hot-reloading:Note that support for
hbase.normalizer.periodis not provided here. Support would need to be implemented generally for theChoresubsystem.