Skip to content

Address comments in HBASE-29081#8115

Open
kgeisz wants to merge 1 commit intoapache:HBASE-29081from
kgeisz:HBASE-29081-onConfigurationChange-refactor
Open

Address comments in HBASE-29081#8115
kgeisz wants to merge 1 commit intoapache:HBASE-29081from
kgeisz:HBASE-29081-onConfigurationChange-refactor

Conversation

@kgeisz
Copy link
Copy Markdown
Contributor

@kgeisz kgeisz commented Apr 22, 2026 •
edited
Loading

This pull request addresses several comments in the review for PR #8044. This PR makes the following changes:

  1. Re-introduced a log message indicating coprocessors are being updated due to a configuration change. This log message has been moved to CoprocessorConfigurationUtil.maybeUpdateCoprocessors(). The message logs the component name (Master/Region Server/Region), as well as the object's this.toString() for clarity. 
2026-04-22T21:22:44,884 INFO  [RpcServer.priority.RWQ.Fifo.read.handler=12,queue=3,port=16000] util.CoprocessorConfigurationUtil: Updating coprocessors for Master hbase-docker,16000,1776892801346 because the configuration has changed
2026-04-22T21:22:44,891 INFO  [RpcServer.priority.RWQ.Fifo.read.handler=12,queue=3,port=16000] util.CoprocessorConfigurationUtil: Config hbase.global.readonly.enabled has been dynamically changed to true for Master hbase-docker,16000,1776892801346
2026-04-22T21:22:44,876 INFO  [RpcServer.priority.RWQ.Fifo.read.handler=14,queue=5,port=16020] util.CoprocessorConfigurationUtil: Updating coprocessors for Region Server hbase-docker,16020,1776892802447 because the configuration has changed
2026-04-22T21:22:44,891 INFO  [RpcServer.priority.RWQ.Fifo.read.handler=14,queue=5,port=16020] util.CoprocessorConfigurationUtil: Config hbase.global.readonly.enabled has been dynamically changed to true for Region Server hbase-docker,16020,1776892802447
2026-04-22T21:22:44,892 INFO  [RpcServer.priority.RWQ.Fifo.read.handler=14,queue=5,port=16020] util.CoprocessorConfigurationUtil: Updating coprocessors for Region hbase:meta,,1.1588230740 because the configuration has changed
2026-04-22T21:22:44,894 INFO  [RpcServer.priority.RWQ.Fifo.read.handler=14,queue=5,port=16020] util.CoprocessorConfigurationUtil: Config hbase.global.readonly.enabled has been dynamically changed to true for Region hbase:meta,,1.1588230740

  1. Read-Only mode is now tracked by checking whether the ReadOnlyController coprocessors are present, rather than using an instance variable in HMaster, HRegionServer, and HRegion. If the coprocessors are present, then read-only mode is assumed to be active.

  2. Removed dynamic configuration for coprocessors, which was introduced in PR HBASE-29236: Add Support for Dynamic Configuration at the Coprocessor Level #6931 and is no longer needed.

  3. The onConfigurationChange() method for HMaster, HRegionServer, and HRegion now use the lambda function's conf variable in maybeUpdateCoprocessors().

  4. Removes extra portion of documentation for HBASE_META_TABLE_SUFFIX.

  5. The lambda function provided as an arg for maybeUpdateCoprocessors() in HMaster/HRegionServer/HRegion.onConfigurationChange() now also updates the class' this.conf instance variable to have the correct loaded coprocessors. This was necessary to prevent unit test failures in TestReadOnlyControllerCoprocessorLoading.

@kgeisz
Copy link
Copy Markdown
Contributor Author

kgeisz commented Apr 22, 2026 •
edited
Loading

Hi @charlesconnell, I have made some changes for the Read-Replica feature (PR #8044) based on your review comments. Can you please take a look when you have time?

@kgeisz
Address coprocessor and dynamic configuration review comments
4899a4e 
Change-Id: Ib7ff9d449f7368153bc67f0bd2d6a7ca591900e8
@kgeisz kgeisz force-pushed the HBASE-29081-onConfigurationChange-refactor branch from 7e57d21 to 4899a4e Compare April 22, 2026 23:57
kgeisz
kgeisz commented Apr 22, 2026
View reviewed changes
Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
public void run() {
try {
installShutdownHook();
registerConfigurationObservers();
Copy link
Copy Markdown
Contributor Author

@kgeisz kgeisz Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was removed in PR #6931, so I am adding it back here

kgeisz
kgeisz commented Apr 23, 2026
View reviewed changes
Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
private void initializeCoprocessorHost(Configuration conf) {
// initialize master side coprocessors before we start handling requests
this.cpHost = new MasterCoprocessorHost(this, conf);
registerConfigurationObservers();
Copy link
Copy Markdown
Contributor Author

@kgeisz kgeisz Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added in PR #6931, so I am removing it again.

sharmaar12
sharmaar12 reviewed Apr 23, 2026
View reviewed changes
Copy link
Copy Markdown

@sharmaar12 sharmaar12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@kgeisz
Copy link
Copy Markdown
Contributor Author

kgeisz commented Apr 23, 2026

The failed unit tests are not relevant. I ran them again locally and they all passed.

kgeisz
kgeisz commented Apr 23, 2026
View reviewed changes
Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Comment on lines +4505 to +4506
CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
Copy link
Copy Markdown
Contributor Author

@kgeisz kgeisz Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to satisfy a unit test in TestReadOnlyControllerCoprocessorLoading. The dynamic configuration worked as expected without this in local deployments or in a docker container setup.

kgeisz
kgeisz commented Apr 23, 2026
View reviewed changes
Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Comment on lines +8998 to +8999
CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
Copy link
Copy Markdown
Contributor Author

@kgeisz kgeisz Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to satisfy a unit test in TestReadOnlyControllerCoprocessorLoading. The dynamic configuration worked as expected without this in local deployments or in a docker container setup.

kgeisz
kgeisz commented Apr 23, 2026
View reviewed changes
Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
Comment on lines +3492 to +3493
CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
Copy link
Copy Markdown
Contributor Author

@kgeisz kgeisz Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to satisfy a unit test in TestReadOnlyControllerCoprocessorLoading. The dynamic configuration worked as expected without this in local deployments or in a docker container setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charlesconnell charlesconnell charlesconnell approved these changes

+1 more reviewer

@sharmaar12 sharmaar12 sharmaar12 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kgeisz @charlesconnell @sharmaar12