sharmaar12 commented on code in PR #7743:
URL: https://github.com/apache/hbase/pull/7743#discussion_r2844791133
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -4422,13 +4430,30 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
+
+ boolean readOnlyMode =
newConf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ if (readOnlyMode) {
+ addReadOnlyCoprocessors(newConf);
+ } else {
Review Comment:
It looks same but there is subtle difference when we consider how we set
readonly value, in the `syncReadOnlyConfigurations()` we are setting
readOnlyMode on this.conf from newConf we receive in OnConfigurationChange. So
logically is translate it to:
```
if (isReadOnlyEnabled(newConf)){
this.conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true);
}
```
If I call syncReadOnlyConfigurations() here then it will translate to:
```
if (isReadOnlyEnabled(this.conf)){
this.conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true);
}
```
It looks like you are checking same value and assigning the same which might
be redundant. I thought it might be confusing.
One Idea I can think of is to extract following code into separate function:
```
if (readOnlyMode) {
addReadOnlyCoprocessors(this.conf);
} else {
removeReadOnlyCoprocessors(this.conf);
}
```
What are your thoughts on this?
--
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]
