Uh oh!
There was an error while loading. Please reload this page.
HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers - #7661
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
wchevreuil
left a comment
There was a problem hiding this comment.
LGTM, just have few nits.
| return Optional.of(this); | ||
| } | ||
| /* ---- ConfigurationObserver Overrides ---- */ |
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 ---- */ |
| return Optional.of(this); | ||
| } | ||
| /* ---- ConfigurationObserver Overrides ---- */ |
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) { |
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| return Optional.of(this); | ||
| } | ||
| /* ---- MasterObserver Overrides ---- */ |
| } | ||
| /* ---- ConfigurationObserver Overrides ---- */ | ||
| public void onConfigurationChange(Configuration conf) { |
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 ---- */ |
| } | ||
| /* ---- ConfigurationObserver Overrides ---- */ | ||
| public void onConfigurationChange(Configuration conf) { |
There was a problem hiding this comment.
nit: remove this comment and add @Override annotation.
| } | ||
| } | ||
| /* ---- RegionObserver Overrides ---- */ |
…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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| private static final Logger LOG = LoggerFactory.getLogger(BulkLoadReadOnlyController.class); | ||
| private volatile boolean globalReadOnlyEnabled; | ||
| private void internalReadOnlyGuard() throws DoNotRetryIOException { |
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?
There was a problem hiding this comment.
I totally agree. A common base class would definitely make sense.
There was a problem hiding this comment.
Thanks for the suggestion. I will create the AbstractReadOnlyController class which implements ConfigurationObserver so that we can move start(), stop(), OnConfigurationChange() and internalReadOnlyGuard().
| internalReadOnlyGuard(); | ||
| RegionObserver.super.preCommitStoreFile(ctx, family, pairs); | ||
| } | ||
| private void manageActiveClusterIdFile(boolean newValue) { |
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) { |
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.
There was a problem hiding this comment.
Agreed. Will update the code accordingly.
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?
sharmaar12
commented
Feb 5, 2026
We don't need to set that individually, instead we will be registering them at the time of initialization depending on the value of the property hbase.global.readonly.supported. This will get introduced as part of HBASE-29756) |
Apache-HBase
commented
Feb 5, 2026
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 5, 2026
💔 -1 overall
This message was automatically generated. |
anmolnar
left a comment
There was a problem hiding this comment.
lgtm. Thanks for the refactoring.
Uh oh!
There was an error while loading. Please reload this page.
…trollers (#7661) * HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers 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. * Addres review Comments
…trollers (#7661) * HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers 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. * Addres review Comments
…trollers (#7661) * HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers 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. * Addres review Comments
…trollers (#7661) * HBASE-29841: Split bulky ReadOnlyController into multiple smaller controllers 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. * Addres review Comments
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.