Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar merged PR #8044: URL: https://github.com/apache/hbase/pull/8044 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4510767446 I've validated the flaky tests and all of them passed on my machine locally. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4499477157 Thanks @ndimiduk for looking. Let me do the rebase and fix the problems. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4496545669 Click through into a failed check and look at the Summary document, https://github.com/apache/hbase/actions/runs/26121214670 . Click on the House and Summary at the top and you'll see an overview. https://github.com/user-attachments/assets/8712d2d9-861e-46e6-8750-1bf46193203b"; /> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4496533538 It's not yetus, the failures are legit -- your merge target is shifting under you. You need to rebase onto master. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4493498220 @ndimiduk Looks like squashing the commits doesn't make any difference. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4491391874 Thank you @charlesconnell ! Let me squash the commits. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4488420562 > I have one unresolved comment still, but other than that no new comments I think we have resolved all the outstanding comments. Please carry on with your review when you have a chance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3260033029
##
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCanStartHBaseInReadOnlyMode.java:
##
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.security.access;
+
+import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_RETRIES_NUMBER;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.*;
+import org.apache.hadoop.hbase.client.*;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.SecurityTests;
+import org.junit.*;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestName;
+
+@Category({ SecurityTests.class, LargeTests.class })
+@SuppressWarnings("deprecation")
+public class TestCanStartHBaseInReadOnlyMode {
+
+ @ClassRule
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3260010958
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##
@@ -382,4 +392,55 @@ public void stop() {
public void logFileSystemState(Logger log) throws IOException {
CommonFSUtils.logFileSystemState(fs, rootdir, log);
}
+
+ private void negotiateActiveClusterSuffixFile(long wait) throws IOException {
Review Comment:
We haven't documented anything yet until we finalize the implementation.
To make things more complicated #7730 is stuck at code reviews and if ever
gets merged, it'll potentially change the implementation radically.
I created a sub-task about the documentation:
https://issues.apache.org/jira/browse/HBASE-30167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4456041350 I have one unresolved comment still, but other than that no new comments -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4434727492 I think that a single squished commit is easiest. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4434431442 > Glad to help. > > FYI, last I looked, this commit history needs some cleanup before you land it on master. We still aim for one commit per JIRA issue. Not sure if you want to just squish it all down so that only the umbrella issue lands in the tree? In that case, be sure to update the fix version on all the subtasks appropriately (i.e., set to the feature branch release version, not the release version). Thanks @ndimiduk for the heads-up, I didn't think about it yet. I can certainly squash the patch is a single commit that's quite straightforward. However, squashing multiple commits of a single sub-task would be a bit more time-consuming, but I think it's still feasible. What do you recommend? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4432344303 Glad to help. FYI, last I looked, this commit history needs some cleanup before you land it on master. We still aim for one commit per JIRA issue. Not sure if you want to just squish it all down so that only the umbrella issue lands in the tree? In that case, be sure to update the fix version on all the subtasks appropriately (i.e., set to the feature branch release version, not the release version). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4431641687 Thanks @ndimiduk for the quick help. I triggered another round of CI. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4419098666 https://issues.apache.org/jira/browse/YETUS-983 is merged. Let's try it again. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4404296719 Trying a fix. https://github.com/apache/yetus/pull/384 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
ndimiduk commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4403457067 I'm pretty sure what you're seeing is an artifact of how Yetus applies each commit in your PR branch (`git apply --binary -v --stat --apply -p1 input.patch`) against a clean checkout of master. Grepping logs, this looks important: For ReadOnlyController.java, the apply log shows 9 stanzas: - Line 544 of stat: 393 + (HBASE-29083 adds the file) - Lines 550, 552, 571, 649, 653, 655, 663, 669: subsequent modifications/re-adds across the feature branch - Line 677: 913 (HBASE-29841 deletion stanza, all -, no +) Somehow it's not tracking changes to this file correctly across the entire history. Looks like this is a known issue in Yetus ([YETUS-983](https://issues.apache.org/jira/browse/YETUS-983)) and has been around since 2020. My best recommendation for now is to squish your history a bit and see what comes of it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4401308952 The yetus checks are failing for legit reasons, for example: ``` [ERROR] /home/runner/work/hbase/hbase/src/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:[889,56] cannot find symbol symbol: method getSuffixFileDataToWrite() location: variable mfs of type org.apache.hadoop.hbase.master.MasterFileSystem ``` and ``` /home/runner/work/hbase/hbase/src/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:[207,70] error: cannot find symbol symbol: variable MASTER_REGION_DIR_NAME location: class MasterRegionFactory ``` I am happy to review again when the build is fixed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4398229906 @charlesconnell Looks like Yetus is making progress with the builds this time. We've finished all the outstanding review items on the patch. Could you please do another quick round of review and approve patch if everything is resolved? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4381830656 > #8182 will help with Yetus failures once it's merged and you rebase Rebased. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4363697165 https://github.com/apache/hbase/pull/8182 will help with Yetus failures once it's merged and you rebase -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4339810881 Hi @charlesconnell, PR https://github.com/apache/hbase/pull/8115 has been merged. Can you please mark the relevant comments as resolved? Thanks -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3157793974
##
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java:
##
@@ -1665,6 +1665,31 @@ public enum OperationStatusCode {
*/
public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
+ /**
+ * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’
Added in HBASE-X to
Review Comment:
This has been addressed in PR https://github.com/apache/hbase/pull/8115.
Can we mark this comment as resolved?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3157793697
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
- this.cpHost = new MasterCoprocessorHost(this, newConf);
+
+boolean originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled;
+
+CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf,
this.isGlobalReadOnlyEnabled,
+ this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY,
this.maintenanceMode,
+ this.toString(), val -> this.isGlobalReadOnlyEnabled = val,
+ conf -> initializeCoprocessorHost(newConf));
Review Comment:
This has been addressed in PR https://github.com/apache/hbase/pull/8115
##
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java:
##
@@ -1665,6 +1665,31 @@ public enum OperationStatusCode {
*/
public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
+ /**
+ * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’
Added in HBASE-X to
Review Comment:
This has been addressed in PR https://github.com/apache/hbase/pull/8115
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3157792905
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##
@@ -102,4 +116,129 @@ private static boolean
hasCoprocessorsConfigured(Configuration conf, String... c
}
return false;
}
+
+ private static List getCoprocessorsFromConfig(Configuration conf,
+String configurationKey) {
+String[] existing = conf.getStrings(configurationKey);
+return existing != null ? new ArrayList<>(Arrays.asList(existing)) : new
ArrayList<>();
+ }
+
+ public static void addCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToAdd) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToAdd) {
+ if (!existing.contains(coprocessor)) {
+existing.add(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ public static void removeCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToRemove) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+if (existing.isEmpty()) {
+ return;
+}
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToRemove) {
+ if (existing.contains(coprocessor)) {
+existing.remove(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ private static List getReadOnlyCoprocessors(String configurationKey)
{
+return switch (configurationKey) {
+ case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> List
+.of(MasterReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> List
+.of(RegionServerReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> List.of(
+RegionReadOnlyController.class.getName(),
BulkLoadReadOnlyController.class.getName(),
+EndpointReadOnlyController.class.getName());
+
+ default -> throw new IllegalArgumentException(
+"Unsupported coprocessor configuration key: " + configurationKey);
+};
+ }
+
+ /**
+ * This method adds or removes relevant ReadOnlyController coprocessors to
the provided
+ * configuration based on whether read-only mode is enabled.
+ * @param conf The up-to-date configuration used to determine
how to handle
+ * coprocessors
+ * @param coprocessorConfKey The configuration key name
+ */
+ public static void syncReadOnlyConfigurations(Configuration conf, String
coprocessorConfKey) {
+boolean isReadOnlyModeEnabled =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+
+List cpList = getReadOnlyCoprocessors(coprocessorConfKey);
+if (isReadOnlyModeEnabled) {
+ CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey,
cpList);
+} else {
+ CoprocessorConfigurationUtil.removeCoprocessors(conf,
coprocessorConfKey, cpList);
+}
+ }
+
+ /**
+ * This method updates the coprocessors on the master, region server, or
region if a change has
+ * been detected. Detected changes include changes in coprocessors or
changes in read-only mode
+ * configuration. If a change is detected, then new coprocessors are loaded
using the provided
+ * reload method. The new value for the read-only config variable is updated
as well.
+ * @param newConf an updated configuration
+ * @param originalIsReadOnlyEnabled the original value for
+ * {@value
HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
+ * @param coprocessorHost the coprocessor host for HMaster,
HRegionServer, or HRegion
+ * @param coprocessorConfKeyconfiguration key used for setting
master, region server, or
+ * region coprocessors
+ * @param isMaintenanceMode whether maintenance mode is active
(mainly for HMaster)
+ * @param instance string value of the instance calling
this method (mainly helps
+ * with tracking region logging)
+ * @param stateSetter lambda function that sets the read-only
instance variable with
+ * an updated value from the config
+ * @param reloadTasklambda function that reloads
coprocessors on the master,
+ * region server, or region
+ */
+ public static void maybeUpdateCoprocessors(Configuration newConf,
+boolean originalIsReadOnlyEnabled, CoprocessorHo
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3157790621
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
Review Comment:
This has been addressed in PR https://github.com/apache/hbase/pull/8115
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4337065334 hey @Apache9 - You might want to take a look at this patch sooner rather than later. Please share your thoughts when you get a chance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
sharmaar12 commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3138809118
##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java:
##
@@ -1153,6 +1153,26 @@ public List getCachedFilesList(ServerName
serverName) throws IOException
return get(admin.getCachedFilesList(serverName));
}
+ @Override
+ public Long refreshMeta() throws IOException {
+return get(admin.refreshMeta());
+ }
+
+ @Override
+ public Long refreshHFiles(final TableName tableName) throws IOException {
+return get(admin.refreshHFiles(tableName));
+ }
+
+ @Override
+ public Long refreshHFiles(final String namespace) throws IOException {
+return get(admin.refreshHFiles(namespace));
+ }
+
+ @Override
+ public Long refreshHFiles() throws IOException {
+return get(admin.refreshHFiles());
+ }
Review Comment:
Submitted PR [#8130 ](https://github.com/apache/hbase/pull/8130)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
sharmaar12 commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3138809118
##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java:
##
@@ -1153,6 +1153,26 @@ public List getCachedFilesList(ServerName
serverName) throws IOException
return get(admin.getCachedFilesList(serverName));
}
+ @Override
+ public Long refreshMeta() throws IOException {
+return get(admin.refreshMeta());
+ }
+
+ @Override
+ public Long refreshHFiles(final TableName tableName) throws IOException {
+return get(admin.refreshHFiles(tableName));
+ }
+
+ @Override
+ public Long refreshHFiles(final String namespace) throws IOException {
+return get(admin.refreshHFiles(namespace));
+ }
+
+ @Override
+ public Long refreshHFiles() throws IOException {
+return get(admin.refreshHFiles());
+ }
Review Comment:
Will update the code accordingly and submit the PR for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3127464564
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
Review Comment:
This log message is now in
`CoprocessorConfigurationUtil.maybeUpdateCoprocessors()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4290492793 We have come across a bug where tables on the replica cluster are not getting new data post-refresh after the read-only flag has been flipped twice. This is being tracked in [HBASE-30090](https://issues.apache.org/jira/browse/HBASE-30090). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3114019176
##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java:
##
@@ -1153,6 +1153,26 @@ public List getCachedFilesList(ServerName
serverName) throws IOException
return get(admin.getCachedFilesList(serverName));
}
+ @Override
+ public Long refreshMeta() throws IOException {
+return get(admin.refreshMeta());
+ }
+
+ @Override
+ public Long refreshHFiles(final TableName tableName) throws IOException {
+return get(admin.refreshHFiles(tableName));
+ }
+
+ @Override
+ public Long refreshHFiles(final String namespace) throws IOException {
+return get(admin.refreshHFiles(namespace));
+ }
+
+ @Override
+ public Long refreshHFiles() throws IOException {
+return get(admin.refreshHFiles());
+ }
Review Comment:
Perhaps these should return primitive `long`s? There is no need for them to
be null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4284664267 No news about the Yetus release -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4270769687 @charlesconnell Any news about the Yetus release? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3102819942
##
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java:
##
@@ -1665,6 +1665,31 @@ public enum OperationStatusCode {
*/
public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
+ /**
+ * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’
Added in HBASE-X to
Review Comment:
@kgeisz Please add this to your upcoming patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4270757515 > I also assumed #7730 will be additions to this PR after we merge this change? Unfortunately we're not getting anywhere with that PR. Given that it's not a strict requirement to complete the feature, I'd like to go forward with this patch at the first place. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
taklwu commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3102591737
##
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java:
##
@@ -1665,6 +1665,31 @@ public enum OperationStatusCode {
*/
public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
+ /**
+ * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’
Added in HBASE-X to
Review Comment:
are we going to fix this ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3090047048
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
Review Comment:
I got rid of this log message because the logic for determining if there was
actually a configuration change was moved to
`CoprocessorConfigurationUtil.maybeUpdateCoprocessors()`. You can find the log
message
[here](https://github.com/apache/hbase/pull/8044/changes#diff-497ef4310988f57855965f564c729f02b1aabb5d3945f4844850d688c622471aR232).
I do think I should be a little more specific in this log message. Right
now, it uses the `toString()` representation of HMaster/HRegionServer/HRegion,
which is a little vague. For example, we see this for master on a local
standalone deployment of HBase:
```
2026-04-15 16:52:51,650 INFO util.CoprocessorConfigurationUtil: Updating
coprocessors for 10.0.0.227,16000,1776296876526 because the configuration has
changed
```
I think it could say `...for Master 10.0.0.227,16000,1776296876526...`
instead of just `...for 10.0.0.227,16000,1776296876526...`
I can also leave out the `toString()` representation entirely if you think
that would be better. I can use the coprocessor configuration key to figure
out whether a master, region server, or region is what is being configured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3090047048
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
Review Comment:
I got rid of this log message because the logic for determining if there was
actually a configuration change was moved to
`CoprocessorConfigurationUtil.maybeUpdateCoprocessors()`. You can find the log
message
[here](https://github.com/apache/hbase/pull/8044/changes#diff-497ef4310988f57855965f564c729f02b1aabb5d3945f4844850d688c622471aR232).
I do think I should be a little more specific in this log message. Right
now, it uses the the `toString()` representation of
HMaster/HRegionServer/HRegion, which is a little vague. For example, we see
this for master on a local standalone deployment of HBase:
```
2026-04-15 16:52:51,650 INFO util.CoprocessorConfigurationUtil: Updating
coprocessors for 10.0.0.227,16000,1776296876526 because the configuration has
changed
```
I think it could say `...for Master 10.0.0.227,16000,1776296876526...`
instead of just `...for 10.0.0.227,16000,1776296876526...`
I can also leave out the `toString()` representation entirely if you think
that would be better. I can use the coprocessor configuration key to figure
out whether a master, region server, or region is what is being configured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3090047048
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
Review Comment:
I got rid of this log message because the logic for determining if there was
actually configuration change has been moved to
`CoprocessorConfigurationUtil.maybeUpdateCoprocessors()`. You can find the log
message
[here](https://github.com/apache/hbase/pull/8044/changes#diff-497ef4310988f57855965f564c729f02b1aabb5d3945f4844850d688c622471aR232).
I do think I should be a little more specific in this log message. Right
now, it uses the the `toString()` representation of
HMaster/HRegionServer/HRegion, which is a little vague. For example, we see
this for master on a local standalone deployment of HBase:
```
2026-04-15 16:52:51,650 INFO util.CoprocessorConfigurationUtil: Updating
coprocessors for 10.0.0.227,16000,1776296876526 because the configuration has
changed
```
I think it could say `...for Master 10.0.0.227,16000,1776296876526...`
instead of just `...for 10.0.0.227,16000,1776296876526...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3089956692
##
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java:
##
@@ -130,6 +132,39 @@ public Set getCoprocessorClassNames() {
return returnValue;
}
+ /**
+ * Used to help make the relevant loaded coprocessors dynamically
configurable by registering them
+ * to the {@link ConfigurationManager}. Coprocessors are considered
"relevant" if they implement
+ * the {@link ConfigurationObserver} interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
registered to
+ */
+ public void registerConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.registerObserver((ConfigurationObserver) foundCp);
+ }
+}
+ }
+
+ /**
+ * Deregisters relevant coprocessors from the {@link ConfigurationManager}.
Coprocessors are
+ * considered "relevant" if they implement the {@link ConfigurationObserver}
interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
deregistered from
+ */
+ public void deregisterConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.deregisterObserver((ConfigurationObserver)
foundCp);
+ }
+}
Review Comment:
This was introduced in PR https://github.com/apache/hbase/pull/6931 when we
first added support for dynamic configuration with the ReadOnlyController. At
that time, we had only one ReadOnlyController class, and it implemented
ConfigurationObserver because we needed to have dynamic configuration
capabilities at the coprocessor level. It looks like this code is not needed
anymore, so I am working on removing it and making sure everything still works
as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3089956692
##
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java:
##
@@ -130,6 +132,39 @@ public Set getCoprocessorClassNames() {
return returnValue;
}
+ /**
+ * Used to help make the relevant loaded coprocessors dynamically
configurable by registering them
+ * to the {@link ConfigurationManager}. Coprocessors are considered
"relevant" if they implement
+ * the {@link ConfigurationObserver} interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
registered to
+ */
+ public void registerConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.registerObserver((ConfigurationObserver) foundCp);
+ }
+}
+ }
+
+ /**
+ * Deregisters relevant coprocessors from the {@link ConfigurationManager}.
Coprocessors are
+ * considered "relevant" if they implement the {@link ConfigurationObserver}
interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
deregistered from
+ */
+ public void deregisterConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.deregisterObserver((ConfigurationObserver)
foundCp);
+ }
+}
Review Comment:
This was introduced in PR https://github.com/apache/hbase/pull/6931 when we
first added support for dynamic configuration with the ReadOnlyController. At
that time, we had only one ReadOnlyController class, and it implemented
ConfigurationObserver. We needed to have dynamic configuration capabilities at
the coprocessor level. It looks like this code is not needed anymore, so I am
working on removing it and making sure everything still works as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3089956692
##
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java:
##
@@ -130,6 +132,39 @@ public Set getCoprocessorClassNames() {
return returnValue;
}
+ /**
+ * Used to help make the relevant loaded coprocessors dynamically
configurable by registering them
+ * to the {@link ConfigurationManager}. Coprocessors are considered
"relevant" if they implement
+ * the {@link ConfigurationObserver} interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
registered to
+ */
+ public void registerConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.registerObserver((ConfigurationObserver) foundCp);
+ }
+}
+ }
+
+ /**
+ * Deregisters relevant coprocessors from the {@link ConfigurationManager}.
Coprocessors are
+ * considered "relevant" if they implement the {@link ConfigurationObserver}
interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
deregistered from
+ */
+ public void deregisterConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.deregisterObserver((ConfigurationObserver)
foundCp);
+ }
+}
Review Comment:
This was introduced in PR https://github.com/apache/hbase/pull/6931 when we
first added support for dynamic configuration with the ReadOnlyController. At
that time, we had only one ReadOnlyController class, and it implemented
ConfigurationObserver. We needed to have dynamic configuration at the
coprocessor level. It looks like this code is not needed anymore, so I am
working on removing it and making sure everything still works as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3089956692
##
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java:
##
@@ -130,6 +132,39 @@ public Set getCoprocessorClassNames() {
return returnValue;
}
+ /**
+ * Used to help make the relevant loaded coprocessors dynamically
configurable by registering them
+ * to the {@link ConfigurationManager}. Coprocessors are considered
"relevant" if they implement
+ * the {@link ConfigurationObserver} interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
registered to
+ */
+ public void registerConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.registerObserver((ConfigurationObserver) foundCp);
+ }
+}
+ }
+
+ /**
+ * Deregisters relevant coprocessors from the {@link ConfigurationManager}.
Coprocessors are
+ * considered "relevant" if they implement the {@link ConfigurationObserver}
interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
deregistered from
+ */
+ public void deregisterConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.deregisterObserver((ConfigurationObserver)
foundCp);
+ }
+}
Review Comment:
This was introduced in PR https://github.com/apache/hbase/pull/6931 when we
first added support for dynamic configuration with the ReadOnlyController. At
that time, we had only one ReadOnlyController class, and it implemented
ConfigurationObserver. It looks like this code is not needed anymore, so I am
working on removing it and making sure everything still works as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3089956692
##
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java:
##
@@ -130,6 +132,39 @@ public Set getCoprocessorClassNames() {
return returnValue;
}
+ /**
+ * Used to help make the relevant loaded coprocessors dynamically
configurable by registering them
+ * to the {@link ConfigurationManager}. Coprocessors are considered
"relevant" if they implement
+ * the {@link ConfigurationObserver} interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
registered to
+ */
+ public void registerConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.registerObserver((ConfigurationObserver) foundCp);
+ }
+}
+ }
+
+ /**
+ * Deregisters relevant coprocessors from the {@link ConfigurationManager}.
Coprocessors are
+ * considered "relevant" if they implement the {@link ConfigurationObserver}
interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
deregistered from
+ */
+ public void deregisterConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.deregisterObserver((ConfigurationObserver)
foundCp);
+ }
+}
Review Comment:
This was introduced in PR https://github.com/apache/hbase/pull/6931 when we
first added support for dynamic configuration with the ReadOnlyController. At
that time, we had only one ReadOnlyController class, and it implemented
ConfigurationObserver. We needed to be able to have dynamic configuration at
the coprocessor level. It looks like this code is not needed anymore, so I am
working on removing it and making sure everything still works as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
kgeisz commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3088418189
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##
@@ -102,4 +116,129 @@ private static boolean
hasCoprocessorsConfigured(Configuration conf, String... c
}
return false;
}
+
+ private static List getCoprocessorsFromConfig(Configuration conf,
+String configurationKey) {
+String[] existing = conf.getStrings(configurationKey);
+return existing != null ? new ArrayList<>(Arrays.asList(existing)) : new
ArrayList<>();
+ }
+
+ public static void addCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToAdd) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToAdd) {
+ if (!existing.contains(coprocessor)) {
+existing.add(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ public static void removeCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToRemove) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+if (existing.isEmpty()) {
+ return;
+}
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToRemove) {
+ if (existing.contains(coprocessor)) {
+existing.remove(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ private static List getReadOnlyCoprocessors(String configurationKey)
{
+return switch (configurationKey) {
+ case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> List
+.of(MasterReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> List
+.of(RegionServerReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> List.of(
+RegionReadOnlyController.class.getName(),
BulkLoadReadOnlyController.class.getName(),
+EndpointReadOnlyController.class.getName());
+
+ default -> throw new IllegalArgumentException(
+"Unsupported coprocessor configuration key: " + configurationKey);
+};
+ }
+
+ /**
+ * This method adds or removes relevant ReadOnlyController coprocessors to
the provided
+ * configuration based on whether read-only mode is enabled.
+ * @param conf The up-to-date configuration used to determine
how to handle
+ * coprocessors
+ * @param coprocessorConfKey The configuration key name
+ */
+ public static void syncReadOnlyConfigurations(Configuration conf, String
coprocessorConfKey) {
+boolean isReadOnlyModeEnabled =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+
+List cpList = getReadOnlyCoprocessors(coprocessorConfKey);
+if (isReadOnlyModeEnabled) {
+ CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey,
cpList);
+} else {
+ CoprocessorConfigurationUtil.removeCoprocessors(conf,
coprocessorConfKey, cpList);
+}
+ }
+
+ /**
+ * This method updates the coprocessors on the master, region server, or
region if a change has
+ * been detected. Detected changes include changes in coprocessors or
changes in read-only mode
+ * configuration. If a change is detected, then new coprocessors are loaded
using the provided
+ * reload method. The new value for the read-only config variable is updated
as well.
+ * @param newConf an updated configuration
+ * @param originalIsReadOnlyEnabled the original value for
+ * {@value
HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
+ * @param coprocessorHost the coprocessor host for HMaster,
HRegionServer, or HRegion
+ * @param coprocessorConfKeyconfiguration key used for setting
master, region server, or
+ * region coprocessors
+ * @param isMaintenanceMode whether maintenance mode is active
(mainly for HMaster)
+ * @param instance string value of the instance calling
this method (mainly helps
+ * with tracking region logging)
+ * @param stateSetter lambda function that sets the read-only
instance variable with
+ * an updated value from the config
+ * @param reloadTasklambda function that reloads
coprocessors on the master,
+ * region server, or region
+ */
+ public static void maybeUpdateCoprocessors(Configuration newConf,
+boolean originalIsReadOnlyEnabled, CoprocessorHo
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3087486602
##
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java:
##
@@ -0,0 +1,217 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.security.access;
+
+import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_RETRIES_NUMBER;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.Delete;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Row;
+import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.master.HMaster;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.SecurityTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Category({ SecurityTests.class, LargeTests.class })
+@SuppressWarnings("deprecation")
+public class TestReadOnlyController {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+HBaseClassTestRule.forClass(TestReadOnlyController.class);
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestReadOnlyController.class);
+ private final HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil();
+ private static final TableName TEST_TABLE =
TableName.valueOf("read_only_test_table");
+ private static final byte[] TEST_FAMILY =
Bytes.toBytes("read_only_table_col_fam");
+ private static HRegionServer hRegionServer;
+ private static HMaster hMaster;
+ private static Configuration conf;
+ private static Connection connection;
+ private static SingleProcessHBaseCluster cluster;
+
+ private static Table testTable;
+ @Rule
+ public TestName name = new TestName();
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Before
+ public void beforeClass() throws Exception {
+conf = TEST_UTIL.getConfiguration();
+
+// Shorten the run time of failed unit tests by limiting retries and the
session timeout
+// threshold
+conf.setInt(HBASE_CLIENT_RETRIES_NUMBER, 1);
+conf.setInt(HConstants.ZK_SESSION_TIMEOUT, 1000);
+
+// Set up test class with Read-Only mode disabled so a table can be created
+conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false);
+
+try {
+ // Start the test cluster
+ cluster = TEST_UTIL.startMiniCluster(1);
+
+ hMaster = cluster.getMaster();
+ hRegionServer =
cluster.getRegionServerThreads().get(0).getRegionServer();
+ connection = ConnectionFactory.createConnection(conf);
+
+ // Create a test table
+ testTable = TEST_UTIL.createTable(TEST_TABLE, TEST_FAMILY);
+} catch (Exception e) {
+ // Delete the created table, and clean up the connection and cluster
before throwing an
+ // exception
+ disableReadOnlyMode();
+ TEST_UTIL.deleteTable(TEST_TABLE);
+ connection.close();
+ TEST_UTIL.shutdownMiniCluster();
+ throw new RuntimeException(e);
+}
+ }
+
+ @After
+ public void afterClass() throws Exception {
+if (connection != null) {
+ connection.close();
+}
+TEST_UTIL.shutdownMiniCluster();
+ }
+
+ private static void enableReadOnlyMode() {
+// Dynamically enable Read-Only mode if it is not active
+if (!isReadOnlyModeEnabled()) {
+ LOG.info("Dynamically enablin
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3087477718
##
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java:
##
@@ -0,0 +1,217 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.security.access;
+
+import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_RETRIES_NUMBER;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.Delete;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Row;
+import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.master.HMaster;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.SecurityTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Category({ SecurityTests.class, LargeTests.class })
+@SuppressWarnings("deprecation")
+public class TestReadOnlyController {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+HBaseClassTestRule.forClass(TestReadOnlyController.class);
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestReadOnlyController.class);
+ private final HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil();
+ private static final TableName TEST_TABLE =
TableName.valueOf("read_only_test_table");
+ private static final byte[] TEST_FAMILY =
Bytes.toBytes("read_only_table_col_fam");
+ private static HRegionServer hRegionServer;
+ private static HMaster hMaster;
+ private static Configuration conf;
+ private static Connection connection;
+ private static SingleProcessHBaseCluster cluster;
+
+ private static Table testTable;
+ @Rule
+ public TestName name = new TestName();
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Before
+ public void beforeClass() throws Exception {
+conf = TEST_UTIL.getConfiguration();
+
+// Shorten the run time of failed unit tests by limiting retries and the
session timeout
+// threshold
+conf.setInt(HBASE_CLIENT_RETRIES_NUMBER, 1);
+conf.setInt(HConstants.ZK_SESSION_TIMEOUT, 1000);
+
+// Set up test class with Read-Only mode disabled so a table can be created
+conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false);
+
+try {
+ // Start the test cluster
+ cluster = TEST_UTIL.startMiniCluster(1);
+
+ hMaster = cluster.getMaster();
+ hRegionServer =
cluster.getRegionServerThreads().get(0).getRegionServer();
+ connection = ConnectionFactory.createConnection(conf);
+
+ // Create a test table
+ testTable = TEST_UTIL.createTable(TEST_TABLE, TEST_FAMILY);
+} catch (Exception e) {
+ // Delete the created table, and clean up the connection and cluster
before throwing an
+ // exception
+ disableReadOnlyMode();
+ TEST_UTIL.deleteTable(TEST_TABLE);
+ connection.close();
+ TEST_UTIL.shutdownMiniCluster();
+ throw new RuntimeException(e);
+}
+ }
+
+ @After
+ public void afterClass() throws Exception {
+if (connection != null) {
+ connection.close();
+}
+TEST_UTIL.shutdownMiniCluster();
+ }
+
+ private static void enableReadOnlyMode() {
+// Dynamically enable Read-Only mode if it is not active
+if (!isReadOnlyModeEnabled()) {
+ LOG.info("Dynamically enablin
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3087452747
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
- this.cpHost = new MasterCoprocessorHost(this, newConf);
+
+boolean originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled;
+
+CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf,
this.isGlobalReadOnlyEnabled,
+ this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY,
this.maintenanceMode,
+ this.toString(), val -> this.isGlobalReadOnlyEnabled = val,
+ conf -> initializeCoprocessorHost(newConf));
+
+if (this.isGlobalReadOnlyEnabled != originalIsReadOnlyEnabled) {
Review Comment:
Similarly, there's and _else_ branch in the `manageActiveClusterIdFile()`
method which tries to remove the suffix file if already exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3087275377 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.access; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Set; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.ActiveClusterSuffix; +import org.apache.hadoop.hbase.Coprocessor; +import org.apache.hadoop.hbase.CoprocessorEnvironment; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.WriteAttemptedOnReadOnlyClusterException; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.exceptions.DeserializationException; +import org.apache.hadoop.hbase.master.MasterFileSystem; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected](HBaseInterfaceAudience.CONFIG) +public abstract class AbstractReadOnlyController implements Coprocessor { + private static final Logger LOG = LoggerFactory.getLogger(AbstractReadOnlyController.class); + + private static final Set writableTables = +Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME); + + public static boolean +isWritableInReadOnlyMode(final ObserverContext c) { +return writableTables.contains(c.getEnvironment().getRegionInfo().getTable()); + } + + public static boolean isWritableInReadOnlyMode(final TableName tableName) { +return writableTables.contains(tableName); + } + + protected void internalReadOnlyGuard() throws WriteAttemptedOnReadOnlyClusterException { +throw new WriteAttemptedOnReadOnlyClusterException("Operation not allowed in Read-Only Mode"); + } + + @Override + public void start(CoprocessorEnvironment env) throws IOException { + } + + @Override + public void stop(CoprocessorEnvironment env) { + } + + public static void manageActiveClusterIdFile(boolean readOnlyEnabled, MasterFileSystem mfs) { +FileSystem fs = mfs.getFileSystem(); +Path rootDir = mfs.getRootDir(); +Path activeClusterFile = new Path(rootDir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME); + +try { + if (readOnlyEnabled) { +// ENABLING READ-ONLY (false -> true), delete the active cluster file. Review Comment: There's an "else" branch as well in this code at line 107. You should not fail fast here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3087275377 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.access; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Set; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.ActiveClusterSuffix; +import org.apache.hadoop.hbase.Coprocessor; +import org.apache.hadoop.hbase.CoprocessorEnvironment; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.WriteAttemptedOnReadOnlyClusterException; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.exceptions.DeserializationException; +import org.apache.hadoop.hbase.master.MasterFileSystem; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected](HBaseInterfaceAudience.CONFIG) +public abstract class AbstractReadOnlyController implements Coprocessor { + private static final Logger LOG = LoggerFactory.getLogger(AbstractReadOnlyController.class); + + private static final Set writableTables = +Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME); + + public static boolean +isWritableInReadOnlyMode(final ObserverContext c) { +return writableTables.contains(c.getEnvironment().getRegionInfo().getTable()); + } + + public static boolean isWritableInReadOnlyMode(final TableName tableName) { +return writableTables.contains(tableName); + } + + protected void internalReadOnlyGuard() throws WriteAttemptedOnReadOnlyClusterException { +throw new WriteAttemptedOnReadOnlyClusterException("Operation not allowed in Read-Only Mode"); + } + + @Override + public void start(CoprocessorEnvironment env) throws IOException { + } + + @Override + public void stop(CoprocessorEnvironment env) { + } + + public static void manageActiveClusterIdFile(boolean readOnlyEnabled, MasterFileSystem mfs) { +FileSystem fs = mfs.getFileSystem(); +Path rootDir = mfs.getRootDir(); +Path activeClusterFile = new Path(rootDir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME); + +try { + if (readOnlyEnabled) { +// ENABLING READ-ONLY (false -> true), delete the active cluster file. Review Comment: There's an "else" branch as well in this code. You should not fail fast here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3087262385
##
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java:
##
@@ -1665,6 +1665,31 @@ public enum OperationStatusCode {
*/
public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
+ /**
+ * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’
Added in HBASE-X to
Review Comment:
I don't think we need that comment. Could be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4252931462 > [DISCUSS] I'm wondered if we should always use junit 5 when merging to master branch, if so, would it be possible to have it in this PR? or at least I want a follow up task on it. I completely agree. I haven't actually noticed that we're still using JUnit 4 somewhere. I tried to migrate every test during the rebasing. Do you have any pointers? I created a sub-task for it: https://issues.apache.org/jira/browse/HBASE-30085 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3087238981
##
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRefreshHFilesFromClient.java:
##
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client;
+
+import static org.junit.Assert.assertTrue;
+
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.NamespaceNotFoundException;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.TableNotFoundException;
+import org.apache.hadoop.hbase.TestRefreshHFilesBase;
+import org.apache.hadoop.hbase.testclassification.ClientTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category({ MediumTests.class, ClientTests.class })
+public class TestRefreshHFilesFromClient extends TestRefreshHFilesBase {
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+HBaseClassTestRule.forClass(TestRefreshHFilesFromClient.class);
+
+ private static final TableName TEST_NONEXISTENT_TABLE =
+TableName.valueOf("testRefreshHFilesNonExistentTable");
+ private static final String TEST_NONEXISTENT_NAMESPACE =
"testRefreshHFilesNonExistentNamespace";
+
+ @Before
+ public void setup() throws Exception {
+baseSetup(false);
+ }
+
+ @After
+ public void tearDown() throws Exception {
+baseTearDown();
+ }
+
+ @Test
+ public void testRefreshHFilesForTable() throws Exception {
+try {
+ // Create table in default namespace
+ createTableAndWait(TEST_TABLE, TEST_FAMILY);
+
+ // RefreshHFiles for table
+ Long procId = admin.refreshHFiles(TEST_TABLE);
+ assertTrue(procId >= 0);
+} catch (Exception e) {
+ Assert.fail("RefreshHFilesForTable Should Not Throw Exception: " + e);
+ throw new RuntimeException(e);
+} finally {
+ // Delete table name post test execution
+ deleteTable(TEST_TABLE);
+}
+ }
+
+ // Not creating table hence refresh should throw exception
+ @Test(expected = TableNotFoundException.class)
Review Comment:
I'm not sure if it's a must, but I believe we should definitely do it in
this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
taklwu commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3082579148 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.access; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Set; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.ActiveClusterSuffix; +import org.apache.hadoop.hbase.Coprocessor; +import org.apache.hadoop.hbase.CoprocessorEnvironment; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.WriteAttemptedOnReadOnlyClusterException; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.exceptions.DeserializationException; +import org.apache.hadoop.hbase.master.MasterFileSystem; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected](HBaseInterfaceAudience.CONFIG) +public abstract class AbstractReadOnlyController implements Coprocessor { + private static final Logger LOG = LoggerFactory.getLogger(AbstractReadOnlyController.class); + + private static final Set writableTables = +Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME); + + public static boolean +isWritableInReadOnlyMode(final ObserverContext c) { +return writableTables.contains(c.getEnvironment().getRegionInfo().getTable()); + } + + public static boolean isWritableInReadOnlyMode(final TableName tableName) { +return writableTables.contains(tableName); + } + + protected void internalReadOnlyGuard() throws WriteAttemptedOnReadOnlyClusterException { +throw new WriteAttemptedOnReadOnlyClusterException("Operation not allowed in Read-Only Mode"); + } + + @Override + public void start(CoprocessorEnvironment env) throws IOException { + } + + @Override + public void stop(CoprocessorEnvironment env) { + } + + public static void manageActiveClusterIdFile(boolean readOnlyEnabled, MasterFileSystem mfs) { +FileSystem fs = mfs.getFileSystem(); +Path rootDir = mfs.getRootDir(); +Path activeClusterFile = new Path(rootDir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME); + +try { + if (readOnlyEnabled) { +// ENABLING READ-ONLY (false -> true), delete the active cluster file. Review Comment: nit: Would the below fast return be better ? ```suggestion if (!readOnlyEnabled) { return; } FileSystem fs = mfs.getFileSystem(); Path rootDir = mfs.getRootDir(); Path activeClusterFile = new Path(rootDir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME); try { // ENABLING READ-ONLY (false -> true), delete the active cluster file. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
taklwu commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3082492374
##
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java:
##
@@ -1665,6 +1665,31 @@ public enum OperationStatusCode {
*/
public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
+ /**
+ * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’
Added in HBASE-X to
Review Comment:
what should `HBASE-X` refer ? should we update this comment before
merging ?
##
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestCanStartHBaseInReadOnlyMode.java:
##
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.security.access;
+
+import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_RETRIES_NUMBER;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.*;
+import org.apache.hadoop.hbase.client.*;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.SecurityTests;
+import org.junit.*;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestName;
+
+@Category({ SecurityTests.class, LargeTests.class })
+@SuppressWarnings("deprecation")
+public class TestCanStartHBaseInReadOnlyMode {
+
+ @ClassRule
Review Comment:
nit: remove if we decide to use junit 5
##
hbase-client/src/main/java/org/apache/hadoop/hbase/ActiveClusterSuffix.java:
##
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+import java.io.IOException;
+import java.util.Objects;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.exceptions.DeserializationException;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.yetus.audience.InterfaceAudience;
+
+import org.apache.hbase.thirdparty.com.google.common.base.Strings;
+
+import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
+import
org.apache.hadoop.hbase.shaded.protobuf.generated.ActiveClusterSuffixProtos;
+
+/**
+ * The read-replica cluster id for this cluster. It is serialized to the
filesystem and up into
+ * zookeeper. This is a container for the id. Also knows how to serialize and
deserialize the
+ * cluster id.
+ */
[email protected]
+public class ActiveClusterSuffix implements ClusterIdFile {
+ private final String cluster_id;
Review Comment:
nit: please follow the camel case in java class, update respectively in this
file and other file.
```suggestion
private final String clusterId;
```
##
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java:
##
@@ -0,0 +1,217 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BA
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
Kota-SH commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3082825468 ## hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RefreshMetaProcedure.java: ## @@ -0,0 +1,480 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.master.procedure; + +import static org.apache.hadoop.hbase.shaded.protobuf.generated.ProcedureProtos.ProcedureState.WAITING_TIMEOUT; + +import java.io.DataInputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.MetaTableAccessor; +import org.apache.hadoop.hbase.NamespaceDescriptor; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Connection; +import org.apache.hadoop.hbase.client.Delete; +import org.apache.hadoop.hbase.client.Mutation; +import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.TableState; +import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer; +import org.apache.hadoop.hbase.procedure2.ProcedureSuspendedException; +import org.apache.hadoop.hbase.procedure2.ProcedureUtil; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.util.CommonFSUtils; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.hadoop.hbase.util.RetryCounter; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.RefreshMetaState; +import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.RefreshMetaStateData; + [email protected] +public class RefreshMetaProcedure extends AbstractStateMachineTableProcedure { + private static final Logger LOG = LoggerFactory.getLogger(RefreshMetaProcedure.class); + private static final String HIDDEN_DIR_PATTERN = "^[._-].*"; + + private List currentRegions; + private List latestRegions; + private List pendingMutations; + private RetryCounter retryCounter; + private static final int MUTATION_BATCH_SIZE = 100; + private List newlyAddedRegions; + private List deletedTables; + + public RefreshMetaProcedure() { +super(); + } + + public RefreshMetaProcedure(MasterProcedureEnv env) { +super(env); + } + + @Override + public TableName getTableName() { +return TableName.META_TABLE_NAME; + } + + @Override + public TableOperationType getTableOperationType() { +return TableOperationType.EDIT; + } + + @Override + protected Flow executeFromState(MasterProcedureEnv env, RefreshMetaState refreshMetaState) { +LOG.info("Executing RefreshMetaProcedure state: {}", refreshMetaState); + +try { + return switch (refreshMetaState) { +case REFRESH_META_INIT -> executeInit(env); +case REFRESH_META_SCAN_STORAGE -> executeScanStorage(env); +case REFRESH_META_PREPARE -> executePrepare(); +case REFRESH_META_APPLY -> executeApply(env); +case REFRESH_META_FOLLOWUP -> executeFollowup(env); +case REFRESH_META_FINISH -> executeFinish(env); +default -> throw new UnsupportedOperationException("Unhandled state: " + refreshMetaState); + }; +} catch (Exception ex) { + LOG.error("Error in RefreshMetaProcedure state {}", refreshMetaState, ex); + setFailure("RefreshMetaProcedure", ex); + return Flow.NO_MORE_STATE; +} + } + + private Flow executeInit(MasterProcedureEnv env) throws IOException {
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
sharmaar12 commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3079240668
##
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RefreshHFilesCallable.java:
##
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.executor.EventType;
+import org.apache.hadoop.hbase.procedure2.BaseRSProcedureCallable;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos;
+
+/**
+ * This is a RegionServer-side callable used by the HBase Master Procedure
framework to perform the
+ * actual HFiles refresh operation on a specific region. It is dispatched from
the
+ * {@link
org.apache.hadoop.hbase.master.procedure.RefreshHFilesRegionProcedure} to the
RegionServer
+ * and executes the logic to refresh store files in each store of the region.
+ */
+
[email protected]
+public class RefreshHFilesCallable extends BaseRSProcedureCallable {
+ private static final Logger LOG =
LoggerFactory.getLogger(RefreshHFilesCallable.class);
+
+ private RegionInfo regionInfo;
+
+ @Override
+ protected byte[] doCall() throws Exception {
+HRegion region = rs.getRegion(regionInfo.getEncodedName());
+LOG.debug("Starting refreshHfiles operation on region {}", region);
+
+try {
+ for (Store store : region.getStores()) {
+store.refreshStoreFiles();
+ }
+} catch (IOException ioe) {
+ LOG.warn("Exception while trying to refresh store files: ", ioe);
+}
Review Comment:
I agree we should send the exception to the user in case of failure. Will be
resolved under [HBASE-30080](https://issues.apache.org/jira/browse/HBASE-30080).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3076388440
##
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java:
##
@@ -130,6 +132,39 @@ public Set getCoprocessorClassNames() {
return returnValue;
}
+ /**
+ * Used to help make the relevant loaded coprocessors dynamically
configurable by registering them
+ * to the {@link ConfigurationManager}. Coprocessors are considered
"relevant" if they implement
+ * the {@link ConfigurationObserver} interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
registered to
+ */
+ public void registerConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.registerObserver((ConfigurationObserver) foundCp);
+ }
+}
+ }
+
+ /**
+ * Deregisters relevant coprocessors from the {@link ConfigurationManager}.
Coprocessors are
+ * considered "relevant" if they implement the {@link ConfigurationObserver}
interface.
+ * @param configurationManager the ConfigurationManager the coprocessors get
deregistered from
+ */
+ public void deregisterConfigurationObservers(ConfigurationManager
configurationManager) {
+Coprocessor foundCp;
+Set coprocessors = this.getCoprocessors();
+for (String cp : coprocessors) {
+ foundCp = this.findCoprocessor(cp);
+ if (foundCp instanceof ConfigurationObserver) {
+configurationManager.deregisterObserver((ConfigurationObserver)
foundCp);
+ }
+}
Review Comment:
The above mentioned callers (HMaster, HRegion, etc.) are the configuration
observers. Since the coprocessors don't have to track the read-only mode flag -
they're either loaded or not loaded - they don't have to observe the
configuration change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3076379044
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##
@@ -102,4 +116,129 @@ private static boolean
hasCoprocessorsConfigured(Configuration conf, String... c
}
return false;
}
+
+ private static List getCoprocessorsFromConfig(Configuration conf,
+String configurationKey) {
+String[] existing = conf.getStrings(configurationKey);
+return existing != null ? new ArrayList<>(Arrays.asList(existing)) : new
ArrayList<>();
+ }
+
+ public static void addCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToAdd) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToAdd) {
+ if (!existing.contains(coprocessor)) {
+existing.add(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ public static void removeCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToRemove) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+if (existing.isEmpty()) {
+ return;
+}
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToRemove) {
+ if (existing.contains(coprocessor)) {
+existing.remove(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ private static List getReadOnlyCoprocessors(String configurationKey)
{
+return switch (configurationKey) {
+ case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> List
+.of(MasterReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> List
+.of(RegionServerReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> List.of(
+RegionReadOnlyController.class.getName(),
BulkLoadReadOnlyController.class.getName(),
+EndpointReadOnlyController.class.getName());
+
+ default -> throw new IllegalArgumentException(
+"Unsupported coprocessor configuration key: " + configurationKey);
+};
+ }
+
+ /**
+ * This method adds or removes relevant ReadOnlyController coprocessors to
the provided
+ * configuration based on whether read-only mode is enabled.
+ * @param conf The up-to-date configuration used to determine
how to handle
+ * coprocessors
+ * @param coprocessorConfKey The configuration key name
+ */
+ public static void syncReadOnlyConfigurations(Configuration conf, String
coprocessorConfKey) {
+boolean isReadOnlyModeEnabled =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+
+List cpList = getReadOnlyCoprocessors(coprocessorConfKey);
+if (isReadOnlyModeEnabled) {
+ CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey,
cpList);
+} else {
+ CoprocessorConfigurationUtil.removeCoprocessors(conf,
coprocessorConfKey, cpList);
+}
+ }
+
+ /**
+ * This method updates the coprocessors on the master, region server, or
region if a change has
+ * been detected. Detected changes include changes in coprocessors or
changes in read-only mode
+ * configuration. If a change is detected, then new coprocessors are loaded
using the provided
+ * reload method. The new value for the read-only config variable is updated
as well.
+ * @param newConf an updated configuration
+ * @param originalIsReadOnlyEnabled the original value for
+ * {@value
HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
+ * @param coprocessorHost the coprocessor host for HMaster,
HRegionServer, or HRegion
+ * @param coprocessorConfKeyconfiguration key used for setting
master, region server, or
+ * region coprocessors
+ * @param isMaintenanceMode whether maintenance mode is active
(mainly for HMaster)
+ * @param instance string value of the instance calling
this method (mainly helps
+ * with tracking region logging)
+ * @param stateSetter lambda function that sets the read-only
instance variable with
+ * an updated value from the config
+ * @param reloadTasklambda function that reloads
coprocessors on the master,
+ * region server, or region
+ */
+ public static void maybeUpdateCoprocessors(Configuration newConf,
+boolean originalIsReadOnlyEnabled, Coprocessor
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3076359902
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
Review Comment:
Yeah, I think I agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3076327278 ## hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RefreshMetaProcedure.java: ## @@ -0,0 +1,480 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.master.procedure; + +import static org.apache.hadoop.hbase.shaded.protobuf.generated.ProcedureProtos.ProcedureState.WAITING_TIMEOUT; + +import java.io.DataInputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.MetaTableAccessor; +import org.apache.hadoop.hbase.NamespaceDescriptor; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Connection; +import org.apache.hadoop.hbase.client.Delete; +import org.apache.hadoop.hbase.client.Mutation; +import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.TableState; +import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer; +import org.apache.hadoop.hbase.procedure2.ProcedureSuspendedException; +import org.apache.hadoop.hbase.procedure2.ProcedureUtil; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.util.CommonFSUtils; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.hadoop.hbase.util.RetryCounter; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.RefreshMetaState; +import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.RefreshMetaStateData; + [email protected] +public class RefreshMetaProcedure extends AbstractStateMachineTableProcedure { + private static final Logger LOG = LoggerFactory.getLogger(RefreshMetaProcedure.class); + private static final String HIDDEN_DIR_PATTERN = "^[._-].*"; + + private List currentRegions; + private List latestRegions; + private List pendingMutations; + private RetryCounter retryCounter; + private static final int MUTATION_BATCH_SIZE = 100; + private List newlyAddedRegions; + private List deletedTables; + + public RefreshMetaProcedure() { +super(); + } + + public RefreshMetaProcedure(MasterProcedureEnv env) { +super(env); + } + + @Override + public TableName getTableName() { +return TableName.META_TABLE_NAME; + } + + @Override + public TableOperationType getTableOperationType() { +return TableOperationType.EDIT; + } + + @Override + protected Flow executeFromState(MasterProcedureEnv env, RefreshMetaState refreshMetaState) { +LOG.info("Executing RefreshMetaProcedure state: {}", refreshMetaState); + +try { + return switch (refreshMetaState) { +case REFRESH_META_INIT -> executeInit(env); +case REFRESH_META_SCAN_STORAGE -> executeScanStorage(env); +case REFRESH_META_PREPARE -> executePrepare(); +case REFRESH_META_APPLY -> executeApply(env); +case REFRESH_META_FOLLOWUP -> executeFollowup(env); +case REFRESH_META_FINISH -> executeFinish(env); +default -> throw new UnsupportedOperationException("Unhandled state: " + refreshMetaState); + }; +} catch (Exception ex) { + LOG.error("Error in RefreshMetaProcedure state {}", refreshMetaState, ex); + setFailure("RefreshMetaProcedure", ex); + return Flow.NO_MORE_STATE; +} + } + + private Flow executeInit(MasterProcedureEnv env) throws IOException {
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3076278916
##
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RefreshHFilesCallable.java:
##
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.executor.EventType;
+import org.apache.hadoop.hbase.procedure2.BaseRSProcedureCallable;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos;
+
+/**
+ * This is a RegionServer-side callable used by the HBase Master Procedure
framework to perform the
+ * actual HFiles refresh operation on a specific region. It is dispatched from
the
+ * {@link
org.apache.hadoop.hbase.master.procedure.RefreshHFilesRegionProcedure} to the
RegionServer
+ * and executes the logic to refresh store files in each store of the region.
+ */
+
[email protected]
+public class RefreshHFilesCallable extends BaseRSProcedureCallable {
+ private static final Logger LOG =
LoggerFactory.getLogger(RefreshHFilesCallable.class);
+
+ private RegionInfo regionInfo;
+
+ @Override
+ protected byte[] doCall() throws Exception {
+HRegion region = rs.getRegion(regionInfo.getEncodedName());
+LOG.debug("Starting refreshHfiles operation on region {}", region);
+
+try {
+ for (Store store : region.getStores()) {
+store.refreshStoreFiles();
+ }
+} catch (IOException ioe) {
+ LOG.warn("Exception while trying to refresh store files: ", ioe);
+}
Review Comment:
I don't think it's needed either. @sharmaar12 could you please comment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4231631779 That will be fixed in the next release of Yetus by https://github.com/apache/yetus/pull/365 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on PR #8044: URL: https://github.com/apache/hbase/pull/8044#issuecomment-4227470368 I have no idea what's wrong with CI builds. Code builds completely fine on my machine with the same JDK version and tests are also passing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3054336616
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##
@@ -102,4 +116,129 @@ private static boolean
hasCoprocessorsConfigured(Configuration conf, String... c
}
return false;
}
+
+ private static List getCoprocessorsFromConfig(Configuration conf,
+String configurationKey) {
+String[] existing = conf.getStrings(configurationKey);
+return existing != null ? new ArrayList<>(Arrays.asList(existing)) : new
ArrayList<>();
+ }
+
+ public static void addCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToAdd) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToAdd) {
+ if (!existing.contains(coprocessor)) {
+existing.add(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ public static void removeCoprocessors(Configuration conf, String
configurationKey,
+List coprocessorsToRemove) {
+List existing = getCoprocessorsFromConfig(conf, configurationKey);
+
+if (existing.isEmpty()) {
+ return;
+}
+
+boolean isModified = false;
+
+for (String coprocessor : coprocessorsToRemove) {
+ if (existing.contains(coprocessor)) {
+existing.remove(coprocessor);
+isModified = true;
+ }
+}
+
+if (isModified) {
+ conf.setStrings(configurationKey, existing.toArray(new String[0]));
+}
+ }
+
+ private static List getReadOnlyCoprocessors(String configurationKey)
{
+return switch (configurationKey) {
+ case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> List
+.of(MasterReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> List
+.of(RegionServerReadOnlyController.class.getName());
+
+ case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> List.of(
+RegionReadOnlyController.class.getName(),
BulkLoadReadOnlyController.class.getName(),
+EndpointReadOnlyController.class.getName());
+
+ default -> throw new IllegalArgumentException(
+"Unsupported coprocessor configuration key: " + configurationKey);
+};
+ }
+
+ /**
+ * This method adds or removes relevant ReadOnlyController coprocessors to
the provided
+ * configuration based on whether read-only mode is enabled.
+ * @param conf The up-to-date configuration used to determine
how to handle
+ * coprocessors
+ * @param coprocessorConfKey The configuration key name
+ */
+ public static void syncReadOnlyConfigurations(Configuration conf, String
coprocessorConfKey) {
+boolean isReadOnlyModeEnabled =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+
+List cpList = getReadOnlyCoprocessors(coprocessorConfKey);
+if (isReadOnlyModeEnabled) {
+ CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey,
cpList);
+} else {
+ CoprocessorConfigurationUtil.removeCoprocessors(conf,
coprocessorConfKey, cpList);
+}
+ }
+
+ /**
+ * This method updates the coprocessors on the master, region server, or
region if a change has
+ * been detected. Detected changes include changes in coprocessors or
changes in read-only mode
+ * configuration. If a change is detected, then new coprocessors are loaded
using the provided
+ * reload method. The new value for the read-only config variable is updated
as well.
+ * @param newConf an updated configuration
+ * @param originalIsReadOnlyEnabled the original value for
+ * {@value
HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
+ * @param coprocessorHost the coprocessor host for HMaster,
HRegionServer, or HRegion
+ * @param coprocessorConfKeyconfiguration key used for setting
master, region server, or
+ * region coprocessors
+ * @param isMaintenanceMode whether maintenance mode is active
(mainly for HMaster)
+ * @param instance string value of the instance calling
this method (mainly helps
+ * with tracking region logging)
+ * @param stateSetter lambda function that sets the read-only
instance variable with
+ * an updated value from the config
+ * @param reloadTasklambda function that reloads
coprocessors on the master,
+ * region server, or region
+ */
+ public static void maybeUpdateCoprocessors(Configuration newConf,
+boolean originalIsReadOnlyEnabled, Copro
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
sharmaar12 commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3055282157 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.access; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Set; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.ActiveClusterSuffix; +import org.apache.hadoop.hbase.Coprocessor; +import org.apache.hadoop.hbase.CoprocessorEnvironment; +import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.exceptions.DeserializationException; +import org.apache.hadoop.hbase.master.MasterFileSystem; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected](HBaseInterfaceAudience.CONFIG) +public abstract class AbstractReadOnlyController implements Coprocessor { + private static final Logger LOG = LoggerFactory.getLogger(AbstractReadOnlyController.class); + + private static final Set writableTables = +Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME); + + public static boolean +isWritableInReadOnlyMode(final ObserverContext c) { +return writableTables.contains(c.getEnvironment().getRegionInfo().getTable()); + } + + public static boolean isWritableInReadOnlyMode(final TableName tableName) { +return writableTables.contains(tableName); + } + + protected void internalReadOnlyGuard() throws DoNotRetryIOException { +throw new DoNotRetryIOException("Operation not allowed in Read-Only Mode"); Review Comment: Noted. Will update code accordingly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3054810871
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -64,10 +70,42 @@ public final class TableName implements
Comparable {
// with NAMESPACE_DELIM as delimiter
public static final String VALID_USER_TABLE_REGEX = "(?:(?:(?:" +
VALID_NAMESPACE_REGEX + "\\"
+ NAMESPACE_DELIM + ")?)" + "(?:" + VALID_TABLE_QUALIFIER_REGEX + "))";
+ public static final String VALID_META_TABLE_SUFFIX_REGEX = "[a-zA-Z0-9]+";
- /** The hbase:meta table's name. */
- public static final TableName META_TABLE_NAME =
-valueOf(NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR, "meta");
+ /**
+ * The name of hbase meta table could either be hbase:meta_xxx or
'hbase:meta' otherwise. Config
+ * hbase.meta.table.suffix will govern the decision of adding suffix to the
habase:meta
+ */
+ public static final TableName META_TABLE_NAME;
+ static {
+Configuration conf = HBaseConfiguration.create();
Review Comment:
We already have a **[refactoring
patch](https://github.com/apache/hbase/pull/7730)** open to make the meta table
name non-static. Until that I think static initialization is a must.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
anmolnar commented on code in PR #8044: URL: https://github.com/apache/hbase/pull/8044#discussion_r3054806351 ## hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java: ## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.access; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Set; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.ActiveClusterSuffix; +import org.apache.hadoop.hbase.Coprocessor; +import org.apache.hadoop.hbase.CoprocessorEnvironment; +import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.exceptions.DeserializationException; +import org.apache.hadoop.hbase.master.MasterFileSystem; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected](HBaseInterfaceAudience.CONFIG) +public abstract class AbstractReadOnlyController implements Coprocessor { + private static final Logger LOG = LoggerFactory.getLogger(AbstractReadOnlyController.class); + + private static final Set writableTables = +Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME); + + public static boolean +isWritableInReadOnlyMode(final ObserverContext c) { +return writableTables.contains(c.getEnvironment().getRegionInfo().getTable()); + } + + public static boolean isWritableInReadOnlyMode(final TableName tableName) { +return writableTables.contains(tableName); + } + + protected void internalReadOnlyGuard() throws DoNotRetryIOException { +throw new DoNotRetryIOException("Operation not allowed in Read-Only Mode"); Review Comment: Makes sense to me. cc @sharmaar12 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-29081: Add HBase Read Replica Cluster feature [hbase]
charlesconnell commented on code in PR #8044:
URL: https://github.com/apache/hbase/pull/8044#discussion_r3054364562
##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -4483,13 +4505,17 @@ public void onConfigurationChange(Configuration
newConf) {
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
-// update region server coprocessor if the configuration has changed.
-if (
- CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost,
newConf,
-CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-) {
- LOG.info("Update the master coprocessor(s) because the configuration has
changed");
- this.cpHost = new MasterCoprocessorHost(this, newConf);
+
+boolean originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled;
+
+CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf,
this.isGlobalReadOnlyEnabled,
+ this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY,
this.maintenanceMode,
+ this.toString(), val -> this.isGlobalReadOnlyEnabled = val,
+ conf -> initializeCoprocessorHost(newConf));
Review Comment:
Use the captured `conf`
##
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java:
##
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.security.access;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.Set;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.ActiveClusterSuffix;
+import org.apache.hadoop.hbase.Coprocessor;
+import org.apache.hadoop.hbase.CoprocessorEnvironment;
+import org.apache.hadoop.hbase.DoNotRetryIOException;
+import org.apache.hadoop.hbase.HBaseInterfaceAudience;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.coprocessor.ObserverContext;
+import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
+import org.apache.hadoop.hbase.exceptions.DeserializationException;
+import org.apache.hadoop.hbase.master.MasterFileSystem;
+import org.apache.hadoop.hbase.master.region.MasterRegionFactory;
+import org.apache.hadoop.hbase.util.FSUtils;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
[email protected](HBaseInterfaceAudience.CONFIG)
+public abstract class AbstractReadOnlyController implements Coprocessor {
+ private static final Logger LOG =
LoggerFactory.getLogger(AbstractReadOnlyController.class);
+
+ private static final Set writableTables =
+Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME);
+
+ public static boolean
+isWritableInReadOnlyMode(final ObserverContext c) {
+return
writableTables.contains(c.getEnvironment().getRegionInfo().getTable());
+ }
+
+ public static boolean isWritableInReadOnlyMode(final TableName tableName) {
+return writableTables.contains(tableName);
+ }
+
+ protected void internalReadOnlyGuard() throws DoNotRetryIOException {
+throw new DoNotRetryIOException("Operation not allowed in Read-Only Mode");
Review Comment:
I think it would be nice to subclass DoNotRetryIOException with something
more specific to this situation, like `WriteAttemptedOnReadOnlyClusterException`
##
hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java:
##
@@ -64,10 +70,42 @@ public final class TableName implements
Comparable {
// with NAMESPACE_DELIM as delimiter
public static final String VALID_USER_TABLE_REGEX = "(?:(?:(?:" +
VALID_NAMESPACE_REGEX + "\\"
+ NAMESPACE_DELIM + ")?)" + "(?:" + VALID_TABLE_QUALIFIER_REGEX + "))";
+ public static final String VALID_META_TABLE_SUFFIX_REGEX = "[a-zA-Z0-9]+";
- /** The hbase:meta table's name. */
- public static final TableName META_TABLE_NAME =
-valueOf(NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR, "meta");
+ /**
+ * The name of hbase meta table could either be hbase:meta_
