HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times

[kgeisz]

https://issues.apache.org/jira/browse/HBASE-30180

Note: Unit tests were generated using AI

• testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle() was generated with Cursor
• testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange() was generated with Claude Opus

Summary

This pull request fixes an issue with the new Read-Replica feature where if you create a table on the active cluster and flip the read-only flag multiple times, then sometimes it is still possible to add data to a table despite the cluster being in read-only mode. This issue was happening due to how the configuration for HRegion was being updated after running update_all_config in the HBase shell.

Key Changes

• Delete the CoprocessorConfigurationUtil.updateCoprocessorListInConf() method since running this in HMaster, HRegionServer, and HRegion's onConfigurationChange() method is redundant.
  • This method was also improperly unsetting the region coprocessors for HRegion's CompoundConfiguration, which was the main cause of HBASE-30180.
• Properly create a new RegionCoprocessorHost object in HRegion.onConfigurationChange() after a dynamic configuration update.
  • The new object was being created with the dynamically updated Configuration object provided as an arg in HRegion.onConfigurationChange(). However, it needs to be created with an updated version of HRegion's this.conf CompoundConfiguration object.
• Add documentation to HMaster, HRegionServer, and HRegion's onConfigurationChange() methods regarding how the Configuration objects work.
  • For HMaster and HRegionServer, the Configuration object provided in their onConfigurationChange() methods is the same object reference as their respective this.conf Configuration objects.
• Rename some variables for more clarity.

More Details

Read-only mode is determined by checking whether the ReadOnly coprocessors are loaded on the cluster. If they are present, then read-only mode is active, and vice-vera if they are not present. HBASE-30180 was being caused by how the CoprocessorConfigurationUtil.updateCoprocessorListInConf() method was unsetting the read-only coprocessors. HRegion's this.conf object is of type CompoundConfiguration. This differs from HMaster and HRegionServer, which use type Configuration. When disabling read-only mode, the updateCoprocessorListInConf() was running unset() on the configuration object, which does not properly unset the config property for CompoundConfigurations. Instead, the property needs to explicitly be set to an empty string.

After further investigation, the updateCoprocessorListInConf() method was redundant and not necessary since the read-only coprocessors were already being added/removed in CoprocessorConfigurationUtil.syncReadOnlyConfigurations(). That is why this PR removes the updateCoprocessorListInConf() entirely.

One additional issue was with how HRegion's this.coprocessorHost object was being updated after a dynamic configuration update. The new RegionCoprocessorHost object created for this.coprocessorHost was using the dynamically updated Configuration object when it should have been using an updated version of HRegion's CompoundConfiguration object. This PR corrects that discrepancy.

[Copilot]

Pull request overview

Fixes HBASE-30180 where toggling the read-only flag multiple times could leave a region writable. The root cause is that HRegion.this.conf is a CompoundConfiguration distinct from the newConf passed to onConfigurationChange(). The previous code synced the read-only coprocessor list onto newConf and built a new RegionCoprocessorHost from newConf rather than from this.conf, missing the table-descriptor layer and leaving stale data. For HMaster/HRegionServer, this.conf and newConf are the same object, so the issue is region-specific.

Changes:

• Overload maybeUpdateCoprocessors to accept a separate confToUpdate, used for syncing read-only coprocessors and passed into the reload lambda.
• Pass this.conf (the CompoundConfiguration) as confToUpdate in HRegion.onConfigurationChange so the new RegionCoprocessorHost is built from the correct config layer; remove the now-unneeded updateCoprocessorListInConf helper.
• Simplify the HMaster/HRegionServer call sites since their this.conf == newConf.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java	Adds confToUpdate parameter to maybeUpdateCoprocessors, removes obsolete updateCoprocessorListInConf helper, gates syncReadOnlyConfigurations on read-only-mode change only.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java	Passes this.conf (CompoundConfiguration) as confToUpdate and uses it for rebuilding RegionCoprocessorHost.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java	Simplifies lambda since this.conf == newConf.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java	Same simplification as HRegionServer.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[taklwu]

nit: Would you be able to come up with a unit test to prevent this error from happening?

[taklwu]

this reminded me about #7514 were facing the same issue, IMO you're doing the right change.

[kgeisz]

The unit test failures are not relevant.

[kgeisz]

I force-pushed another commit to fix a poorly formatted comment. We can run the tests again if needed. However, all relevant tests passed on the last run.

I also updated the PR description.

[kgeisz]

Two more additions:

• Gave HRegion.getConfiguration() the @RestrictedApi annotation
• Added unit test: testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange()
  • This test ensures HRegion's this.coprocessorHost variable is updated with a new RegionCoprocessorHost object that uses a CompoundConfiguration object in its constructor (rather than a Configuration object).