From 04a7367131b54be653c28d8585a457f248efd9c2 Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Wed, 11 Mar 2026 17:29:02 +0530 Subject: [PATCH 1/2] Remove unused variable --- .../hbase/security/access/AbstractReadOnlyController.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java index c8c1837b21b7..ac430e5b9294 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java @@ -30,7 +30,6 @@ import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.master.MasterFileSystem; -import org.apache.hadoop.hbase.master.MasterServices; import org.apache.hadoop.hbase.util.FSUtils; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; @@ -38,7 +37,6 @@ @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG) public abstract class AbstractReadOnlyController implements Coprocessor { - private MasterServices masterServices; private static final Logger LOG = LoggerFactory.getLogger(AbstractReadOnlyController.class); protected void internalReadOnlyGuard() throws DoNotRetryIOException { From 6f80c51a4c074516769054e0f3ca06c9df16f198 Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Wed, 11 Mar 2026 17:34:08 +0530 Subject: [PATCH 2/2] HBASE-29960 java.lang.IllegalStateException: Should not call create writer on secondary replicas or in read-only mode --- .../StoreFileTrackerBase.java | 26 ++- .../access/AbstractReadOnlyController.java | 24 ++- .../access/RegionReadOnlyController.java | 50 +++-- .../DummyStoreFileTrackerForReadOnlyMode.java | 29 ++- .../TestStoreFileTrackerBaseReadOnlyMode.java | 194 ++++++++++++++---- .../TestReadOnlyControllerRegionObserver.java | 138 +++++++++++++ 6 files changed, 379 insertions(+), 82 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java index 29d8e0bcb485..dd05f486e6f3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java @@ -51,6 +51,7 @@ import org.apache.hadoop.hbase.regionserver.StoreFileInfo; import org.apache.hadoop.hbase.regionserver.StoreFileWriter; import org.apache.hadoop.hbase.regionserver.StoreUtils; +import org.apache.hadoop.hbase.security.access.AbstractReadOnlyController; import org.apache.hadoop.hbase.util.CommonFSUtils; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.HFileArchiveUtil; @@ -84,14 +85,24 @@ protected StoreFileTrackerBase(Configuration conf, boolean isPrimaryReplica, Sto this.ctx = ctx; } + private boolean isReadOnlyEnabled() { + return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, + HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); + } + + private boolean isNonWritableTableWhenReadOnlyMode() { + return isReadOnlyEnabled() + && !AbstractReadOnlyController.isWritableInReadOnlyMode(ctx.getTableName()); + } + @Override public final List load() throws IOException { - return doLoadStoreFiles(!isPrimaryReplica || isReadOnlyEnabled()); + return doLoadStoreFiles(!isPrimaryReplica || isNonWritableTableWhenReadOnlyMode()); } @Override public final void add(Collection newFiles) throws IOException { - if (isPrimaryReplica && !isReadOnlyEnabled()) { + if (isPrimaryReplica && !isNonWritableTableWhenReadOnlyMode()) { doAddNewStoreFiles(newFiles); } } @@ -99,14 +110,14 @@ public final void add(Collection newFiles) throws IOException { @Override public final void replace(Collection compactedFiles, Collection newFiles) throws IOException { - if (isPrimaryReplica && !isReadOnlyEnabled()) { + if (isPrimaryReplica && !isNonWritableTableWhenReadOnlyMode()) { doAddCompactionResults(compactedFiles, newFiles); } } @Override public final void set(List files) throws IOException { - if (isPrimaryReplica && !isReadOnlyEnabled()) { + if (isPrimaryReplica && !isNonWritableTableWhenReadOnlyMode()) { doSetStoreFiles(files); } } @@ -141,7 +152,7 @@ private HFileContext createFileContext(Compression.Algorithm compression, @Override public final StoreFileWriter createWriter(CreateStoreFileWriterParams params) throws IOException { - if (!isPrimaryReplica || isReadOnlyEnabled()) { + if (!isPrimaryReplica || isNonWritableTableWhenReadOnlyMode()) { throw new IllegalStateException( "Should not call create writer on secondary replicas or in read-only mode"); } @@ -387,11 +398,6 @@ protected void archiveStoreFiles(List storeFiles) throws IOException storeFiles); } - private boolean isReadOnlyEnabled() { - return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, - HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); - } - /** * For primary replica, we will call load once when opening a region, and the implementation could * choose to do some cleanup work. So here we use {@code readOnly} to indicate that whether you diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java index ac430e5b9294..737f90fbb0f1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java @@ -20,6 +20,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Set; import org.apache.commons.io.IOUtils; import org.apache.hadoop.fs.FSDataInputStream; import org.apache.hadoop.fs.FileSystem; @@ -29,7 +30,11 @@ 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.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; @@ -39,6 +44,18 @@ 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"); } @@ -74,10 +91,10 @@ public static void manageActiveClusterIdFile(boolean readOnlyEnabled, MasterFile + "Actual data: {}, Expected data: {}", new String(actualClusterFileData), new String(expectedClusterFileData)); } - } catch (FileNotFoundException e) { + } catch (FileNotFoundException e) { LOG.debug("Active cluster file does not exist at: {}. No need to delete.", activeClusterFile); - } catch (IOException e) { + } catch (IOException e) { LOG.error( "Failed to delete active cluster file: {}. " + "Read-only flag will be updated, but file system state is inconsistent.", @@ -87,7 +104,8 @@ public static void manageActiveClusterIdFile(boolean readOnlyEnabled, MasterFile // DISABLING READ-ONLY (true -> false), create the active cluster file id file int wait = mfs.getConfiguration().getInt(HConstants.THREAD_WAKE_FREQUENCY, 10 * 1000); if (!fs.exists(activeClusterFile)) { - FSUtils.setActiveClusterSuffix(fs, rootDir, mfs.computeAndSetSuffixFileDataToWrite(), wait); + FSUtils.setActiveClusterSuffix(fs, rootDir, mfs.computeAndSetSuffixFileDataToWrite(), + wait); } else { LOG.debug("Active cluster file already exists at: {}. No need to create it again.", activeClusterFile); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/RegionReadOnlyController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/RegionReadOnlyController.java index 411b4459f129..6c6ef1e6a953 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/RegionReadOnlyController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/RegionReadOnlyController.java @@ -23,7 +23,6 @@ import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.CompareOperator; import org.apache.hadoop.hbase.HBaseInterfaceAudience; -import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.Append; import org.apache.hadoop.hbase.client.CheckAndMutate; import org.apache.hadoop.hbase.client.CheckAndMutateResult; @@ -60,10 +59,6 @@ public class RegionReadOnlyController extends AbstractReadOnlyController implements RegionCoprocessor, RegionObserver { - private boolean isOnMeta(final ObserverContext c) { - return TableName.isMetaTableName(c.getEnvironment().getRegionInfo().getTable()); - } - @Override public Optional getRegionObserver() { return Optional.of(this); @@ -72,7 +67,7 @@ public Optional getRegionObserver() { @Override public void preFlushScannerOpen(ObserverContext c, Store store, ScanOptions options, FlushLifeCycleTracker tracker) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preFlushScannerOpen(c, store, options, tracker); @@ -81,7 +76,7 @@ public void preFlushScannerOpen(ObserverContext c, FlushLifeCycleTracker tracker) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preFlush(c, tracker); @@ -90,7 +85,7 @@ public void preFlush(final ObserverContext c, Store store, InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preFlush(c, store, scanner, tracker); @@ -123,7 +118,7 @@ public InternalScanner preMemStoreCompactionCompact( public void preCompactSelection(ObserverContext c, Store store, List candidates, CompactionLifeCycleTracker tracker) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preCompactSelection(c, store, candidates, tracker); @@ -133,7 +128,7 @@ public void preCompactSelection(ObserverContext c, Store store, ScanType scanType, ScanOptions options, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preCompactScannerOpen(c, store, scanType, options, tracker, request); @@ -143,7 +138,7 @@ public void preCompactScannerOpen(ObserverContext c, Store store, InternalScanner scanner, ScanType scanType, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCompact(c, store, scanner, scanType, tracker, request); @@ -152,7 +147,7 @@ public InternalScanner preCompact(ObserverContext c, Put put, WALEdit edit, Durability durability) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.prePut(c, put, edit, durability); @@ -161,7 +156,7 @@ public void prePut(ObserverContext c, Pu @Override public void prePut(ObserverContext c, Put put, WALEdit edit) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.prePut(c, put, edit); @@ -170,7 +165,7 @@ public void prePut(ObserverContext c, Pu @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit, Durability durability) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preDelete(c, delete, edit, durability); @@ -179,7 +174,7 @@ public void preDelete(ObserverContext c, @Override public void preDelete(ObserverContext c, Delete delete, WALEdit edit) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preDelete(c, delete, edit); @@ -188,7 +183,7 @@ public void preDelete(ObserverContext c, @Override public void preBatchMutate(ObserverContext c, MiniBatchOperationInProgress miniBatchOp) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } RegionObserver.super.preBatchMutate(c, miniBatchOp); @@ -198,7 +193,7 @@ public void preBatchMutate(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPut(c, row, family, qualifier, op, comparator, put, @@ -208,7 +203,7 @@ public boolean preCheckAndPut(ObserverContext c, byte[] row, Filter filter, Put put, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPut(c, row, filter, put, result); @@ -219,7 +214,7 @@ public boolean preCheckAndPutAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, @@ -230,7 +225,7 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndPutAfterRowLock( ObserverContext c, byte[] row, Filter filter, Put put, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndPutAfterRowLock(c, row, filter, put, result); @@ -240,7 +235,7 @@ public boolean preCheckAndPutAfterRowLock( public boolean preCheckAndDelete(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, @@ -250,7 +245,7 @@ public boolean preCheckAndDelete(ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDelete(c, row, filter, delete, result); @@ -261,7 +256,7 @@ public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, @@ -272,7 +267,7 @@ public boolean preCheckAndDeleteAfterRowLock( public boolean preCheckAndDeleteAfterRowLock( ObserverContext c, byte[] row, Filter filter, Delete delete, boolean result) throws IOException { - if (!isOnMeta(c)) { + if (!isWritableInReadOnlyMode(c)) { internalReadOnlyGuard(); } return RegionObserver.super.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); @@ -339,7 +334,7 @@ public Result preIncrementAfterRowLock(ObserverContext ctx, RegionInfo info, Path edits) throws IOException { - if (!isOnMeta(ctx)) { + if (!isWritableInReadOnlyMode(ctx)) { internalReadOnlyGuard(); } RegionObserver.super.preReplayWALs(ctx, info, edits); @@ -362,8 +357,9 @@ public void preCommitStoreFile(ObserverContext ctx, WALKey key, WALEdit edit) throws IOException { - // Only allow this operation for meta table - if (!TableName.isMetaTableName(key.getTableName())) { + // Only allow this operation for whitelisted table. + // See {@link writableTables set} for details. + if (!isWritableInReadOnlyMode(key.getTableName())) { internalReadOnlyGuard(); } RegionObserver.super.preWALAppend(ctx, key, edit); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DummyStoreFileTrackerForReadOnlyMode.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DummyStoreFileTrackerForReadOnlyMode.java index 5f8268b3ac7c..712199ade99d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DummyStoreFileTrackerForReadOnlyMode.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DummyStoreFileTrackerForReadOnlyMode.java @@ -22,16 +22,41 @@ import java.util.Collections; import java.util.List; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.RegionInfoBuilder; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.regionserver.StoreContext; import org.apache.hadoop.hbase.regionserver.StoreFileInfo; +import org.mockito.Mockito; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class DummyStoreFileTrackerForReadOnlyMode extends StoreFileTrackerBase { + private static final Logger LOG = + LoggerFactory.getLogger(DummyStoreFileTrackerForReadOnlyMode.class); + private boolean readOnlyUsed = false; private boolean compactionExecuted = false; private boolean addExecuted = false; private boolean setExecuted = false; - public DummyStoreFileTrackerForReadOnlyMode(Configuration conf, boolean isPrimaryReplica) { - super(conf, isPrimaryReplica, null); + private static StoreContext buildStoreContext(Configuration conf, TableName tableName) { + RegionInfo regionInfo = RegionInfoBuilder.newBuilder(tableName).build(); + HRegionFileSystem hfs = Mockito.mock(HRegionFileSystem.class); + try { + Mockito.when(hfs.getRegionInfo()).thenReturn(regionInfo); + Mockito.when(hfs.getFileSystem()).thenReturn(FileSystem.get(conf)); + } catch (IOException e) { + LOG.error("Failed to get FileSystem for StoreContext creation", e); + } + return StoreContext.getBuilder().withRegionFileSystem(hfs).build(); + } + + public DummyStoreFileTrackerForReadOnlyMode(Configuration conf, boolean isPrimaryReplica, + TableName tableName) { + super(conf, isPrimaryReplica, buildStoreContext(conf, tableName)); } @Override diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerBaseReadOnlyMode.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerBaseReadOnlyMode.java index 8a1cee55832a..6bd83c67c9e0 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerBaseReadOnlyMode.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerBaseReadOnlyMode.java @@ -17,13 +17,15 @@ */ package org.apache.hadoop.hbase.regionserver.storefiletracker; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import java.util.Collections; import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TestRefreshHFilesBase; import org.apache.hadoop.hbase.master.procedure.TestRefreshHFilesProcedureWithReadOnlyConf; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; import org.apache.hadoop.hbase.regionserver.CreateStoreFileWriterParams; import org.apache.hadoop.hbase.testclassification.RegionServerTests; import org.apache.hadoop.hbase.testclassification.SmallTests; @@ -41,6 +43,8 @@ public class TestStoreFileTrackerBaseReadOnlyMode extends TestRefreshHFilesBase public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestRefreshHFilesProcedureWithReadOnlyConf.class); + TableName tableName = TableName.valueOf("TestStoreFileTrackerBaseReadOnlyMode"); + @Before public void setup() throws Exception { // When true is passed only setup for readonly property is done. @@ -53,28 +57,52 @@ public void tearDown() throws Exception { baseTearDown(); } - @Test - public void testLoadReadOnlyWhenGlobalReadOnlyEnabled() throws Exception { + private void verifyLoadInReadOnlyMode(boolean readOnlyMode, TableName table, + boolean expectReadOnly, String msg) throws Exception { try { - setReadOnlyMode(true); - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + setReadOnlyMode(readOnlyMode); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true, table); tracker.load(); - assertTrue("Tracker should be in read-only mode", tracker.wasReadOnlyLoad()); - } catch (Exception e) { - throw new RuntimeException(e); + assertEquals(msg, expectReadOnly, tracker.wasReadOnlyLoad()); } finally { setReadOnlyMode(false); } } @Test - public void testReplaceSkippedWhenGlobalReadOnlyEnabled() throws Exception { + public void testLoadNonWritableTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyLoadInReadOnlyMode(true, tableName, true, + "For non-writable tables, the doLoadStoreFiles() should get called with readOnly=true"); + } + + @Test + public void testLoadMetaTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyLoadInReadOnlyMode(true, TableName.META_TABLE_NAME, false, + "As meta table is always writable, the doLoadStoreFiles should not get called with readOnly=false even if readonly mode is enabled"); + } + + @Test + public void testLoadMasterStoreTableWhenGlobalReadOnlyEnabled() throws Exception { + // As master:store table is always writable, the doLoadStoreFiles should not get called with + // readOnly=true + verifyLoadInReadOnlyMode(true, MasterRegionFactory.TABLE_NAME, false, + "As master:store table is always writable, the doLoadStoreFiles should not get called with readOnly=false even if readonly mode is enabled"); + } + + @Test + public void testLoadWhenGlobalReadOnlyDisabled() throws Exception { + // When readonly mode is disabled, then it should not interfere with normal functionality + verifyLoadInReadOnlyMode(false, tableName, false, + "As readonly mode is not set, the doLoadStoreFiles() should get called with readOnly=false"); + } + + private void verifyReplaceInReadOnlyMode(boolean readOnlyMode, TableName table, + boolean expectCompactionExecuted, String msg) throws Exception { try { - setReadOnlyMode(true); - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + setReadOnlyMode(readOnlyMode); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true, table); tracker.replace(Collections.emptyList(), Collections.emptyList()); - assertFalse("Compaction should not be executed in readonly mode", - tracker.wasCompactionExecuted()); + assertEquals(msg, expectCompactionExecuted, tracker.wasCompactionExecuted()); } catch (Exception e) { throw new RuntimeException(e); } finally { @@ -83,19 +111,36 @@ public void testReplaceSkippedWhenGlobalReadOnlyEnabled() throws Exception { } @Test - public void testReplaceExecutedWhenWritable() throws Exception { - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); - tracker.replace(Collections.emptyList(), Collections.emptyList()); - assertTrue("Compaction should run when not readonly", tracker.wasCompactionExecuted()); + public void testReplaceSkippedForNonWritableTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyReplaceInReadOnlyMode(true, tableName, false, + "Compaction should not be executed for non-writable table in readonly mode"); } @Test - public void testAddSkippedWhenGlobalReadOnlyEnabled() throws Exception { + public void testReplaceExecutedForMetaTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyReplaceInReadOnlyMode(true, TableName.META_TABLE_NAME, true, + "Compaction should be executed for meta table in readonly mode"); + } + + @Test + public void testReplaceExecutedForMasterStoreTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyReplaceInReadOnlyMode(true, MasterRegionFactory.TABLE_NAME, true, + "Compaction should be executed for master:store table in readonly mode"); + } + + @Test + public void testReplaceExecutedWhenGlobalReadOnlyDisabled() throws Exception { + verifyReplaceInReadOnlyMode(false, tableName, true, + "Compaction should be executed for any table when readonly mode is disabled"); + } + + private void verifyAddInReadOnlyMode(boolean readOnlyMode, TableName table, + boolean expectAddExecuted, String msg) throws Exception { try { - setReadOnlyMode(true); - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + setReadOnlyMode(readOnlyMode); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true, table); tracker.add(Collections.emptyList()); - assertFalse("Add should not be executed in readonly mode", tracker.wasAddExecuted()); + assertEquals(msg, expectAddExecuted, tracker.wasAddExecuted()); } catch (Exception e) { throw new RuntimeException(e); } finally { @@ -104,19 +149,36 @@ public void testAddSkippedWhenGlobalReadOnlyEnabled() throws Exception { } @Test - public void testAddExecutedWhenWritable() throws Exception { - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); - tracker.add(Collections.emptyList()); - assertTrue("Add should run when not readonly", tracker.wasAddExecuted()); + public void testAddSkippedForNonWritableTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyAddInReadOnlyMode(true, tableName, false, + "Add should not be executed for non-writable table in readonly mode"); + } + + @Test + public void testAddExecutedForMetaTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyAddInReadOnlyMode(true, TableName.META_TABLE_NAME, true, + "Add should be executed for meta table in readonly mode"); } @Test - public void testSetSkippedWhenGlobalReadOnlyEnabled() throws Exception { + public void testAddExecutedForMasterStoreTableWhenGlobalReadOnlyEnabled() throws Exception { + verifyAddInReadOnlyMode(true, MasterRegionFactory.TABLE_NAME, true, + "Add should be executed for master:store table in readonly mode"); + } + + @Test + public void testAddExecutedWhenGlobalReadOnlyDisabled() throws Exception { + verifyAddInReadOnlyMode(false, tableName, true, + "Add should be executed for any table when readonly mode is disabled"); + } + + private void verifySetInReadOnlyMode(boolean readOnlyMode, TableName table, + boolean expectSetExecuted, String msg) throws Exception { try { - setReadOnlyMode(true); - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + setReadOnlyMode(readOnlyMode); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true, table); tracker.set(Collections.emptyList()); - assertFalse("Set should not be executed in readonly mode", tracker.wasSetExecuted()); + assertEquals(msg, expectSetExecuted, tracker.wasSetExecuted()); } catch (Exception e) { throw new RuntimeException(e); } finally { @@ -125,22 +187,74 @@ public void testSetSkippedWhenGlobalReadOnlyEnabled() throws Exception { } @Test - public void testSetExecutedWhenWritable() throws Exception { - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); - tracker.set(Collections.emptyList()); - assertTrue("Set should run when not readonly", tracker.wasSetExecuted()); + public void testSetSkippedForNonWritableTableWhenGlobalReadOnlyEnabled() throws Exception { + verifySetInReadOnlyMode(true, tableName, false, + "Set should not be executed for non-writable table in readonly mode"); } - @Test(expected = IllegalStateException.class) - public void testCreateWriterThrowExceptionWhenGlobalReadOnlyEnabled() throws Exception { + @Test + public void testSetExecutedForMetaTableWhenGlobalReadOnlyEnabled() throws Exception { + verifySetInReadOnlyMode(true, TableName.META_TABLE_NAME, true, + "Set should be executed for meta table in readonly mode"); + } + + @Test + public void testSetExecutedForMasterStoreTableWhenGlobalReadOnlyEnabled() throws Exception { + verifySetInReadOnlyMode(true, MasterRegionFactory.TABLE_NAME, true, + "Set should be executed for master:store table in readonly mode"); + } + + @Test + public void testSetExecutedWhenGlobalReadOnlyDisabled() throws Exception { + verifySetInReadOnlyMode(false, tableName, true, + "Set should be executed for any table when readonly mode is disabled"); + } + + private CreateStoreFileWriterParams createParams() { + return CreateStoreFileWriterParams.create().maxKeyCount(4).isCompaction(false) + .includeMVCCReadpoint(true).includesTag(false).shouldDropBehind(false); + } + + private void assertIllegalStateThrown(TableName tableName) throws Exception { + try { + setReadOnlyMode(true); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true, tableName); + tracker.createWriter(createParams()); + fail("Expected IllegalStateException"); + } finally { + setReadOnlyMode(false); + } + } + + private void assertNoIllegalStateThrown(TableName tableName) throws Exception { try { setReadOnlyMode(true); - tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); - CreateStoreFileWriterParams params = CreateStoreFileWriterParams.create().maxKeyCount(4) - .isCompaction(false).includeMVCCReadpoint(true).includesTag(false).shouldDropBehind(false); - tracker.createWriter(params); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true, tableName); + try { + tracker.createWriter(createParams()); + } catch (IllegalStateException e) { + fail("Should not throw IllegalStateException for table " + tableName); + } catch (Exception e) { + // Ignore other exceptions as they are not the focus of this test + } } finally { setReadOnlyMode(false); } } + + @Test(expected = IllegalStateException.class) + public void testCreateWriterThrowExceptionWhenGlobalReadOnlyEnabled() throws Exception { + assertIllegalStateThrown(tableName); + } + + @Test + public void testCreateWriterNoExceptionMetaTableWhenGlobalReadOnlyEnabled() throws Exception { + assertNoIllegalStateThrown(TableName.META_TABLE_NAME); + } + + @Test + public void testCreateWriterNoExceptionMasterStoreTableWhenGlobalReadOnlyEnabled() + throws Exception { + assertNoIllegalStateThrown(MasterRegionFactory.TABLE_NAME); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java index d7176b900552..f56a88399531 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerRegionObserver.java @@ -40,6 +40,7 @@ import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; import org.apache.hadoop.hbase.filter.ByteArrayComparable; import org.apache.hadoop.hbase.filter.Filter; +import org.apache.hadoop.hbase.master.region.MasterRegionFactory; import org.apache.hadoop.hbase.regionserver.FlushLifeCycleTracker; import org.apache.hadoop.hbase.regionserver.InternalScanner; import org.apache.hadoop.hbase.regionserver.MiniBatchOperationInProgress; @@ -175,6 +176,10 @@ private void mockOperationForMetaTable() { when(regionInfo.getTable()).thenReturn(TableName.META_TABLE_NAME); } + private void mockOperationMasterStoreTable() { + when(regionInfo.getTable()).thenReturn(MasterRegionFactory.TABLE_NAME); + } + @Test(expected = DoNotRetryIOException.class) public void testPreFlushV1ReadOnlyException() throws IOException { regionReadOnlyController.preFlush(c, flushLifeCycleTracker); @@ -186,6 +191,12 @@ public void testPreFlushV1ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preFlush(c, flushLifeCycleTracker); } + @Test + public void testPreFlushV1ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preFlush(c, flushLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreFlushV2ReadOnlyException() throws IOException { regionReadOnlyController.preFlush(c, store, scanner, flushLifeCycleTracker); @@ -197,6 +208,12 @@ public void testPreFlushV2ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preFlush(c, store, scanner, flushLifeCycleTracker); } + @Test + public void testPreFlushV2ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preFlush(c, store, scanner, flushLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreFlushScannerOpenReadOnlyException() throws IOException { regionReadOnlyController.preFlushScannerOpen(c, store, options, flushLifeCycleTracker); @@ -208,6 +225,12 @@ public void testPreFlushScannerOpenReadOnlyMetaNoException() throws IOException regionReadOnlyController.preFlushScannerOpen(c, store, options, flushLifeCycleTracker); } + @Test + public void testPreFlushScannerOpenReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preFlushScannerOpen(c, store, options, flushLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreMemStoreCompactionReadOnlyException() throws IOException { regionReadOnlyController.preMemStoreCompaction(c, store); @@ -234,6 +257,12 @@ public void testPreCompactSelectionReadOnlyMetaNoException() throws IOException regionReadOnlyController.preCompactSelection(c, store, candidates, compactionLifeCycleTracker); } + @Test + public void testPreCompactSelectionReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCompactSelection(c, store, candidates, compactionLifeCycleTracker); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCompactScannerOpenReadOnlyException() throws IOException { regionReadOnlyController.preCompactScannerOpen(c, store, scanType, options, @@ -247,6 +276,13 @@ public void testPreCompactScannerOpenReadOnlyMetaNoException() throws IOExceptio compactionLifeCycleTracker, compactionRequest); } + @Test + public void testPreCompactScannerOpenReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCompactScannerOpen(c, store, scanType, options, + compactionLifeCycleTracker, compactionRequest); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCompactReadOnlyException() throws IOException { regionReadOnlyController.preCompact(c, store, scanner, scanType, compactionLifeCycleTracker, @@ -260,6 +296,13 @@ public void testPreCompactReadOnlyMetaNoException() throws IOException { compactionRequest); } + @Test + public void testPreCompactReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCompact(c, store, scanner, scanType, compactionLifeCycleTracker, + compactionRequest); + } + @Test(expected = DoNotRetryIOException.class) public void testPrePutV1ReadOnlyException() throws IOException { regionReadOnlyController.prePut(c, put, edit); @@ -271,6 +314,12 @@ public void testPrePutV1ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.prePut(c, put, edit); } + @Test + public void testPrePutV1ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.prePut(c, put, edit); + } + @Test(expected = DoNotRetryIOException.class) public void testPrePutV2ReadOnlyException() throws IOException { regionReadOnlyController.prePut(c, put, edit, durability); @@ -282,6 +331,12 @@ public void testPrePutV2ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.prePut(c, put, edit, durability); } + @Test + public void testPrePutV2ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.prePut(c, put, edit, durability); + } + @Test(expected = DoNotRetryIOException.class) public void testPreDeleteV1ReadOnlyException() throws IOException { regionReadOnlyController.preDelete(c, delete, edit); @@ -293,6 +348,12 @@ public void testPreDeleteV1ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preDelete(c, delete, edit); } + @Test + public void testPreDeleteV1ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preDelete(c, delete, edit); + } + @Test(expected = DoNotRetryIOException.class) public void testPreDeleteV2ReadOnlyException() throws IOException { regionReadOnlyController.preDelete(c, delete, edit, durability); @@ -304,6 +365,12 @@ public void testPreDeleteV2ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preDelete(c, delete, edit, durability); } + @Test + public void testPreDeleteV2ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preDelete(c, delete, edit, durability); + } + @Test(expected = DoNotRetryIOException.class) public void testPreBatchMutateReadOnlyException() throws IOException { regionReadOnlyController.preBatchMutate(c, miniBatchOp); @@ -315,6 +382,12 @@ public void testPreBatchMutateReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preBatchMutate(c, miniBatchOp); } + @Test + public void testPreBatchMutateReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preBatchMutate(c, miniBatchOp); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutV1ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); @@ -326,6 +399,12 @@ public void testPreCheckAndPutV1ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); } + @Test + public void testPreCheckAndPutV1ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndPut(c, row, family, qualifier, op, comparator, put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutV2ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndPut(c, row, filter, put, result); @@ -337,6 +416,12 @@ public void testPreCheckAndPutV2ReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preCheckAndPut(c, row, filter, put, result); } + @Test + public void testPreCheckAndPutV2ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndPut(c, row, filter, put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutAfterRowLockV1ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, comparator, @@ -350,6 +435,13 @@ public void testPreCheckAndPutAfterRowLockV1ReadOnlyMetaNoException() throws IOE put, result); } + @Test + public void testPreCheckAndPutAfterRowLockV1ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndPutAfterRowLock(c, row, family, qualifier, op, comparator, + put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndPutAfterRowLockV2ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndPutAfterRowLock(c, row, filter, put, result); @@ -361,6 +453,12 @@ public void testPreCheckAndPutAfterRowLockV2ReadOnlyMetaNoException() throws IOE regionReadOnlyController.preCheckAndPutAfterRowLock(c, row, filter, put, result); } + @Test + public void testPreCheckAndPutAfterRowLockV2ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndPutAfterRowLock(c, row, filter, put, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteV1ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, @@ -374,6 +472,13 @@ public void testPreCheckAndDeleteV1ReadOnlyMetaNoException() throws IOException result); } + @Test + public void testPreCheckAndDeleteV1ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndDelete(c, row, family, qualifier, op, comparator, delete, + result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteV2ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndDelete(c, row, filter, delete, result); @@ -385,6 +490,12 @@ public void testPreCheckAndDeleteV2ReadOnlyMetaNoException() throws IOException regionReadOnlyController.preCheckAndDelete(c, row, filter, delete, result); } + @Test + public void testPreCheckAndDeleteV2ReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndDelete(c, row, filter, delete, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteAfterRowLockV1ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, @@ -398,6 +509,14 @@ public void testPreCheckAndDeleteAfterRowLockV1ReadOnlyMetaNoException() throws comparator, delete, result); } + @Test + public void testPreCheckAndDeleteAfterRowLockV1ReadOnlyMasterStoreNoException() + throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndDeleteAfterRowLock(c, row, family, qualifier, op, + comparator, delete, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); @@ -409,6 +528,13 @@ public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyMetaNoException() throws regionReadOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); } + @Test + public void testPreCheckAndDeleteAfterRowLockV2ReadOnlyMasterStoreNoException() + throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preCheckAndDeleteAfterRowLock(c, row, filter, delete, result); + } + @Test(expected = DoNotRetryIOException.class) public void testPreCheckAndMutateReadOnlyException() throws IOException { regionReadOnlyController.preCheckAndMutate(c, checkAndMutate, checkAndMutateResult); @@ -460,6 +586,12 @@ public void testPreReplayWALsReadOnlyMetaNoException() throws IOException { regionReadOnlyController.preReplayWALs(ctx, info, edits); } + @Test + public void testPreReplayWALsReadOnlyMasterStoreNoException() throws IOException { + mockOperationMasterStoreTable(); + regionReadOnlyController.preReplayWALs(ctx, info, edits); + } + @Test(expected = DoNotRetryIOException.class) public void testPreBulkLoadHFileReadOnlyException() throws IOException { regionReadOnlyController.preBulkLoadHFile(ctx, familyPaths); @@ -480,4 +612,10 @@ public void testPreWALAppendReadOnlyMetaNoException() throws IOException { when(key.getTableName()).thenReturn(TableName.META_TABLE_NAME); regionReadOnlyController.preWALAppend(ctx, key, edit); } + + @Test + public void testPreWALAppendReadOnlyMasterStoreNoException() throws IOException { + when(key.getTableName()).thenReturn(MasterRegionFactory.TABLE_NAME); + regionReadOnlyController.preWALAppend(ctx, key, edit); + } }