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-XXXXX to
Review Comment:
what should `HBASE-XXXXX` 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" 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()) {
Review Comment:
```suggestion
if (!ConfigurationUtil.isReadOnlyModeEnabled(conf)) {
```
##########
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:
nit: need some work for junit 4 to junit 5, but as mentioned, we at least
need to know if this is a must in master branch.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java:
##########
@@ -442,6 +449,13 @@ private Optional<CompactionContext>
selectCompaction(HRegion region, HStore stor
LOG.info(String.format("User has disabled compactions"));
return Optional.empty();
}
+
+ // Should not allow compaction if cluster is in read-only mode
+ if (isReadOnlyEnabled()) {
+ LOG.info(String.format("Compaction request skipped as read-only mode is
on"));
Review Comment:
```suggestion
LOG.info("Compaction request skipped as read-only mode is on");
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java:
##########
@@ -344,6 +345,12 @@ protected void requestCompactionInternal(HRegion region,
HStore store, String wh
return;
}
+ // Should not allow compaction if cluster is in read-only mode
+ if (isReadOnlyEnabled()) {
+ LOG.info("Ignoring compaction request for " + region + ",because
read-only mode is on.");
Review Comment:
nit about log message
```suggestion
LOG.info("Ignoring compaction request for {} because read-only mode is
on.", region);
```
##########
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")
Review Comment:
nit: should we use tag of junit 5 before merging ? you may need to fix it
for other classes as well. but if we decide to fix it later, please file JIRA
and link it with https://issues.apache.org/jira/browse/HBASE-23671 ?
```suggestion
@Tag(SecurityTests.TAG)
@Tag(LargeTests.TAG)
```
##########
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;
+ private final String suffix;
+
+ public static class Parser implements
ClusterIdFileParser<ActiveClusterSuffix> {
+
+ @Override
+ public String getFileName() {
+ return HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME;
+ }
+
+ /**
+ * Parse the serialized representation of the {@link ActiveClusterSuffix}
+ * @param bytes A pb serialized {@link ActiveClusterSuffix} instance with
pb magic prefix
+ * @return An instance of {@link ActiveClusterSuffix} made from
<code>bytes</code>
+ * @see #toByteArray()
+ */
+ @Override
+ public ActiveClusterSuffix parseFrom(byte[] bytes) throws
DeserializationException {
+ if (ProtobufUtil.isPBMagicPrefix(bytes)) {
+ int pblen = ProtobufUtil.lengthOfPBMagic();
+ ActiveClusterSuffixProtos.ActiveClusterSuffix.Builder builder =
+ ActiveClusterSuffixProtos.ActiveClusterSuffix.newBuilder();
+ ActiveClusterSuffixProtos.ActiveClusterSuffix cs = null;
+ try {
+ ProtobufUtil.mergeFrom(builder, bytes, pblen, bytes.length - pblen);
+ cs = builder.build();
+ } catch (IOException e) {
+ throw new DeserializationException(e);
+ }
+ return convert(cs);
+ } else {
+ // Presume it was written out this way, the old way.
+ return new ActiveClusterSuffix(Bytes.toString(bytes));
+ }
+ }
+
+ @Override
+ public ActiveClusterSuffix readString(String input) {
+ return new ActiveClusterSuffix(input);
+ }
+ }
+
+ public ActiveClusterSuffix(final String ci, final String suffix) {
+ this.cluster_id = ci;
Review Comment:
```suggestion
public ActiveClusterSuffix(final String clusterId, final String suffix) {
this.clusterId = clusterId;
```
##########
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:
nit: do we have any JIRA for documenting the switch to an active cluster
from a read-replica by using the `hbase.meta.table.suffix`?
##########
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 {
+ this.activeClusterSuffix = ActiveClusterSuffix.fromConfig(conf,
getClusterId());
+ if (!isReadOnlyModeEnabled(conf)) {
+ try {
+ // verify the contents against the config set
+ ActiveClusterSuffix acs =
+ FSUtils.getClusterIdFile(fs, rootdir, new
ActiveClusterSuffix.Parser());
+ if (acs == null) {
+ throw new FileNotFoundException("[Read-replica feature] Active
Cluster Suffix File "
+ + new Path(rootdir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME) +
" not found");
+ }
+ LOG.debug(
+ "Negotiating active cluster suffix file. File {} : File Suffix {} :
Configured suffix {} : Cluster ID : {}",
+ new Path(rootdir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME), acs,
activeClusterSuffix,
+ getClusterId());
+ // Suffix file exists and we're in read/write mode. Content should
match.
+ if (!this.activeClusterSuffix.equals(acs)) {
+ // throw error
+ LOG.info(
Review Comment:
nit: should the mismatch message be error level? or should it be at least a
warning?
```suggestion
LOG.error(
```
##########
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<TableName> writableTables =
+ Set.of(TableName.META_TABLE_NAME, MasterRegionFactory.TABLE_NAME);
+
+ public static boolean
+ isWritableInReadOnlyMode(final ObserverContext<? extends
RegionCoprocessorEnvironment> 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 below fast return 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.
```
##########
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 {
+ this.activeClusterSuffix = ActiveClusterSuffix.fromConfig(conf,
getClusterId());
+ if (!isReadOnlyModeEnabled(conf)) {
Review Comment:
```suggestion
if (!ConfigurationUtil.isReadOnlyModeEnabled(conf)) {
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java:
##########
@@ -857,6 +871,11 @@ public boolean isCompactionsEnabled() {
return compactionsEnabled;
}
+ private boolean isReadOnlyEnabled() {
+ return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ }
+
Review Comment:
```suggestion
```
##########
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:
nit: if we don't need to manage the active cluster Id for the non-readonly
cluster, maybe we should skip early.
```suggestion
if (this.isGlobalReadOnlyEnabled != originalIsReadOnlyEnabled &&
this.isGlobalReadOnlyEnabled) {
```
##########
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<String> 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<String> coprocessorsToAdd) {
+ List<String> 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<String> coprocessorsToRemove) {
+ List<String> 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<String> 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);
Review Comment:
```suggestion
boolean isReadOnlyModeEnabled =
ConfigurationUtil.isReadOnlyModeEnabled(conf);
```
##########
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 {
+ this.activeClusterSuffix = ActiveClusterSuffix.fromConfig(conf,
getClusterId());
+ if (!isReadOnlyModeEnabled(conf)) {
+ try {
+ // verify the contents against the config set
+ ActiveClusterSuffix acs =
+ FSUtils.getClusterIdFile(fs, rootdir, new
ActiveClusterSuffix.Parser());
+ if (acs == null) {
+ throw new FileNotFoundException("[Read-replica feature] Active
Cluster Suffix File "
+ + new Path(rootdir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME) +
" not found");
+ }
+ LOG.debug(
+ "Negotiating active cluster suffix file. File {} : File Suffix {} :
Configured suffix {} : Cluster ID : {}",
+ new Path(rootdir, HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME), acs,
activeClusterSuffix,
+ getClusterId());
+ // Suffix file exists and we're in read/write mode. Content should
match.
+ if (!this.activeClusterSuffix.equals(acs)) {
+ // throw error
+ LOG.info(
+ "[Read-replica feature] Another cluster is running in active
(read-write) mode on this "
+ + "storage location. Active cluster ID: {}, This cluster ID {}.
Rootdir location {} ",
+ acs, activeClusterSuffix, rootdir);
+ throw new IOException("Cannot start master, because another cluster
is running in active "
+ + "(read-write) mode on this storage location. Active Cluster Id:
" + acs
+ + ", This cluster Id: " + activeClusterSuffix);
+ }
+ LOG.info(
+ "[Read-replica feature] This is the active cluster on this storage
location with cluster id: {}",
+ activeClusterSuffix);
+ } catch (FileNotFoundException fnfe) {
+ // We're in read/write mode, but suffix file missing, let's create it
+ FSUtils.setClusterIdFile(fs, rootdir,
HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME,
+ activeClusterSuffix, wait);
+ LOG.info("[Read-replica feature] Created Active cluster suffix file:
{}, with content: {}",
+ HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME, activeClusterSuffix);
+ }
+ } else {
+ // This is a read-only cluster, don't care about suffix file
+ LOG.info("[Read-replica feature] Replica cluster is being started in
Read Only Mode");
+ }
+ }
+
+ public ActiveClusterSuffix getActiveClusterSuffix() {
+ return activeClusterSuffix;
+ }
+
+ private boolean isReadOnlyModeEnabled(Configuration conf) {
+ return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ }
Review Comment:
```suggestion
```
##########
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 enabling Read-Only mode by setting {} to true",
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true);
+ notifyObservers();
+ }
+ }
+
+ private static void disableReadOnlyMode() {
+ // Dynamically disable Read-Only mode if it is active
+ if (isReadOnlyModeEnabled()) {
Review Comment:
```suggestion
if (ConfigurationUtil.isReadOnlyModeEnabled(conf)) {
```
##########
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 enabling Read-Only mode by setting {} to true",
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true);
+ notifyObservers();
+ }
+ }
+
+ private static void disableReadOnlyMode() {
+ // Dynamically disable Read-Only mode if it is active
+ if (isReadOnlyModeEnabled()) {
+ LOG.info("Dynamically disabling Read-Only mode by setting {} to false",
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false);
+ notifyObservers();
+ }
+ }
+
+ private static boolean isReadOnlyModeEnabled() {
+ return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+ }
Review Comment:
```suggestion
```
--
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]
