Skip to content

HBASE-29756: Programmatically register related co-processor during initialization#7743

Open
sharmaar12 wants to merge 2 commits intoapache:HBASE-29081from
sharmaar12:prog_reg_cop
Open

HBASE-29756: Programmatically register related co-processor during initialization#7743
sharmaar12 wants to merge 2 commits intoapache:HBASE-29081from
sharmaar12:prog_reg_cop

Conversation

@sharmaar12
Copy link

No description provided.

@sharmaar12 sharmaar12 marked this pull request as ready for review February 12, 2026 15:15
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

Please see my comment.

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

Choose a reason for hiding this comment

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

I think there's an issue with this approach.

  1. 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.
  2. 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.

Copy link
Contributor

@kgeisz kgeisz left a comment

Choose a reason for hiding this comment

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

LGTM aside from @anmolnar's comment

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants