anmolnar commented on code in PR #7743:
URL: https://github.com/apache/hbase/pull/7743#discussion_r2843722861
##########
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 {
+ // Needed as safety measure in case the coprocessors are added in
hbase-site.xml manually and
+ // the user toggles the read only mode on and off.
Review Comment:
This makes me a bit concerned. Since we deliver this feature, adding
read-only coprocessors to hbase-site.xml will be invalid configuration. Why
would like to implement this workaround for it?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -4527,6 +4552,52 @@ private void setQuotasObserver(Configuration conf) {
}
}
+ private void addReadOnlyCoprocessors(Configuration conf) {
+ String[] masterCoprocs =
conf.getStrings(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
+ // If already present, do nothing
+ if (
+ masterCoprocs != null
+ &&
Arrays.asList(masterCoprocs).contains(MasterReadOnlyController.class.getName())
+ ) {
+ return;
+ }
+
+ final int length = null == masterCoprocs ? 0 : masterCoprocs.length;
+ String[] updatedCoprocs = new String[length + 1];
+ if (length > 0) {
+ System.arraycopy(masterCoprocs, 0, updatedCoprocs, 0,
masterCoprocs.length);
+ }
+ updatedCoprocs[length] = MasterReadOnlyController.class.getName();
Review Comment:
Instead of juggling with arrays here, you can just use generic type
`ArrayList` and call `toArray()` method at the end.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -1292,6 +1306,80 @@ RegionEventDescriptor.EventType.REGION_CLOSE,
getRegionInfo(), mvcc.getReadPoint
}
}
+ private String[] append(String[] original, String value) {
+ String[] updated = new String[original.length + 1];
+ System.arraycopy(original, 0, updated, 0, original.length);
+ updated[original.length] = value;
+ return updated;
+ }
Review Comment:
Again. Generic type `ArrayList` has been invented to provide higher level
convenience methods over low level arrays. Feel free to use it and convert it
back to array[] once you finished the processing.
`StringBuilder` could be another useful high level abstraction for the logic
here.
##########
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:
You've repeated the same logic that you already implemented in
`syncReadOnlyConfigurations()` method. Call it here instead.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -1086,6 +1088,12 @@ private void finishActiveMasterInitialization() throws
IOException, InterruptedE
if (!maintenanceMode) {
startupTaskGroup.addTask("Initializing master coprocessors");
setQuotasObserver(conf);
+ if (
+ conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT)
Review Comment:
You might want to extract this to separate method:
```java
public boolean isGlobalReadonlyEnabled(Configuration conf) {
return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
}
```
--
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]
