HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers#7661
Open
sharmaar12 wants to merge 1 commit intoapache:HBASE-29081from
Open
HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers#7661sharmaar12 wants to merge 1 commit intoapache:HBASE-29081from
sharmaar12 wants to merge 1 commit intoapache:HBASE-29081from
Conversation
sharmaar12
force-pushed
the
dyn_cop_reg
branch
from
360a366 to
6e3b3e4
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Jan 23, 2026
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
LGTM, just have few nits.
| return Optional.of(this); | ||
| } | ||
|
|
||
| /* ---- ConfigurationObserver Overrides ---- */ |
Contributor
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.globalReadOnlyEnabled); | ||
| } | ||
|
|
||
| /* ---- BulkLoadObserver Overrides ---- */ |
Contributor
There was a problem hiding this comment.
nit: remove this comment.
| return Optional.of(this); | ||
| } | ||
|
|
||
| /* ---- ConfigurationObserver Overrides ---- */ |
Contributor
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| if (!isOnMeta(c)) { | ||
| internalReadOnlyGuard(); | ||
| /* ---- ConfigurationObserver Overrides ---- */ | ||
| public void onConfigurationChange(Configuration conf) { |
Contributor
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| return Optional.of(this); | ||
| } | ||
|
|
||
| /* ---- MasterObserver Overrides ---- */ |
Contributor
There was a problem hiding this comment.
nit: remove this comment.
| } | ||
|
|
||
| /* ---- ConfigurationObserver Overrides ---- */ | ||
| public void onConfigurationChange(Configuration conf) { |
Contributor
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.globalReadOnlyEnabled); | ||
| } | ||
|
|
||
| /* ---- RegionServerObserver Overrides ---- */ |
Contributor
There was a problem hiding this comment.
nit: remove this comment.
| } | ||
|
|
||
| /* ---- ConfigurationObserver Overrides ---- */ | ||
| public void onConfigurationChange(Configuration conf) { |
Contributor
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| } | ||
| } | ||
|
|
||
| /* ---- RegionObserver Overrides ---- */ |
Contributor
There was a problem hiding this comment.
nit: remove this comment.
…trollers Currently we have created a single ReadOnlyController which needs to get added as coprocessor for master, region and region server. In this task we will be breaking ReadOnlyController into multiple smaller controller to avoid unnecessarily adding methods which are not relevant for particular role for example, master copocessor should only register methods which may run on master and not on region or region server.
sharmaar12
force-pushed
the
dyn_cop_reg
branch
from
6e3b3e4 to
15fcc92
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
kgeisz
approved these changes
Jan 28, 2026
| private static final Logger LOG = LoggerFactory.getLogger(BulkLoadReadOnlyController.class); | ||
| private volatile boolean globalReadOnlyEnabled; | ||
|
|
||
| private void internalReadOnlyGuard() throws DoNotRetryIOException { |
Contributor
There was a problem hiding this comment.
I noticed there are come common methods among these different types of ReadOnlyController classes. The ones I noticed are internalReadOnlyGuard(), start(), stop(), and onConfigurationChange(). I see the MasterReadOnlyController class has some different implementations for start() and onConfigurationChange(), but the other methods seem roughly the same for each class. Do you think it would make sense to have some kind of ReadOnlyControllerBase class that defines these methods and MasterReadOnlyController can override anything that needs to be implemented differently?
Contributor
There was a problem hiding this comment.
I totally agree. A common base class would definitely make sense.
| internalReadOnlyGuard(); | ||
| RegionObserver.super.preCommitStoreFile(ctx, family, pairs); | ||
| } | ||
| private void manageActiveClusterIdFile(boolean newValue) { |
Contributor
There was a problem hiding this comment.
nit: Can we change newValue to something like isEnablingReadOnly (or similar)? When I first saw newValue, it made me think this newValue was being used to assign its value to something. However, it looks like it's just used to determine what we are doing with the active cluster file.
| public void onConfigurationChange(Configuration conf) { | ||
| boolean maybeUpdatedConfValue = conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, | ||
| HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); | ||
| if (this.globalReadOnlyEnabled != maybeUpdatedConfValue) { |
Contributor
There was a problem hiding this comment.
Is there a reason why this implementation of onConfigurationChange() has the if (this.globalReadOnlyEnabled != maybeUpdatedConfValue) block while the other implementations of this method don't? We may want this if block in each method to prevent extra logging when nothing was actually changed.
anmolnar
approved these changes
Feb 4, 2026
Contributor
anmolnar
left a comment
anmolnar
left a comment
There was a problem hiding this comment.
Overall looks good to me. Please address @kgeisz 's suggestion.
The split looks good, but are we going to set this up in HBase. Do we need to set the controllers individually in the config or are you planning to implement an on/off switch to make it easier in a later patch?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently we have created a single ReadOnlyController which needs to get added as coprocessor for master, region and region server. In this task we will be breaking ReadOnlyController into multiple smaller controller to avoid unnecessarily adding methods which are not relevant for particular role for example, master copocessor should only register methods which may run on master and not on region or region server.