HBASE-29756: Programmatically register related co-processor during initialization by sharmaar12 · Pull Request #7743 · apache/hbase

sharmaar12 · 2026-02-12T15:15:03Z

No description provided.

…itialization

anmolnar

Please see my comment.

anmolnar · 2026-02-12T16:58:04Z

...server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java

+      LOG.info("Updated {} readOnlyEnabled={}", this.getClass().getName(),
+        readOnlyEnabledFromConfig);
+      if (this.masterServices != null) {
+        manageActiveClusterIdFile(readOnlyEnabledFromConfig);


I think there's an issue with this approach.

We talked about briefly that the local globalReadOnlyEnabled is not needed anymore, because coprocs will be loaded only when R/O mode is enabled. So, I think you should remove the field in this patch.

As a consequence this method will always be called with readOnlyEnabledFromConfig == true and the active cluster id file will never be removed.

I suggest to make this method static and call it from HMaster at line 4440. You don't need to resolve the coproc, just call the static method directly with the new configuration and do the management of cluster id file.

Noted and Done.

kgeisz

LGTM aside from @anmolnar's comment

…DFile static

sharmaar12 · 2026-02-20T19:21:16Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+      addReadOnlyCoprocessors(this.baseConf);
+      addReadOnlyCoprocessors(this.conf);
+    } else {
+      removeReadOnlyCoprocessors(this.baseConf);


We need to add this to introduce the REGION_COPROCESSOR_CONF_KEY property in the conf object, which is instance of CompoundConfiguration. This is needed because if the property is not present as a local copy in CompoundConfiguration then updating it using setStrings does not work so though Coprocessors are not needed still the property must be set to empty collection.

sharmaar12 · 2026-02-20T19:23:18Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+    this.conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, readOnlyMode);
+    // If readonly is true then add the coprocessor of master
+    if (readOnlyMode) {
+      addReadOnlyCoprocessors(this.baseConf);


We must sync both conf as well as baseConf object because baseConf is used when new table/region is created and synching just conf will make the new table to use initial value with which the cluster started. This is also important when region split happens as in the split baseConf gets used instead of conf object.

sharmaar12 · 2026-02-20T19:24:34Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+      // remove the coprocessors
+      // Also conf.unset will not have any effect on CompoundConfiguration, unlike with Hmaster and
+      // HRegionServer
+      conf.setStrings(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, new String[0]);


We have to explicitly use setStrings instead of conf.unset() because unset does not work with CompoundConfiguration (shallow copy), as setting it to null or using unset has no effect on CompoundConfiguration.

anmolnar

See my initial review feedback.

anmolnar · 2026-02-24T00:00:27Z

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

+        conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+          HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT)


You might want to extract this to separate method:

public boolean isGlobalReadonlyEnabled(Configuration conf) { return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); }

anmolnar · 2026-02-24T00:07:42Z

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

+      // 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.


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?

anmolnar · 2026-02-24T00:09:18Z

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

+    if (readOnlyMode) {
+      addReadOnlyCoprocessors(newConf);
+    } else {


You've repeated the same logic that you already implemented in syncReadOnlyConfigurations() method. Call it here instead.

anmolnar · 2026-02-24T00:13:11Z

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

+    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();


Instead of juggling with arrays here, you can just use generic type ArrayList and call toArray() method at the end.

anmolnar · 2026-02-24T00:16:09Z

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

+  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;
+  }


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.

sharmaar12 marked this pull request as ready for review February 12, 2026 15:15

HBASE-29756: Programmatically register related co-processor during in…

2909891

…itialization

sharmaar12 force-pushed the prog_reg_cop branch from b61c399 to 2909891 Compare February 12, 2026 16:16

Apply Spotless

f82af29

anmolnar requested changes Feb 12, 2026

View reviewed changes

kgeisz approved these changes Feb 13, 2026

View reviewed changes

sharmaar12 requested a review from anmolnar February 20, 2026 19:09

Remove the cached globalReadOnlyMode variable and make manageclusterI…

f3889b3

…DFile static

sharmaar12 force-pushed the prog_reg_cop branch from b04b6cb to f3889b3 Compare February 20, 2026 19:18

sharmaar12 commented Feb 20, 2026

View reviewed changes

anmolnar reviewed Feb 24, 2026

View reviewed changes

		conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
		HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT)

		// 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.

Comments

Conversation

sharmaar12 commented Feb 12, 2026

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgeisz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharmaar12 Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kgeisz left a comment •

edited

Loading

sharmaar12 Feb 20, 2026 •

edited

Loading