From 7935421a71912b51b00eaa7ce28c56baf5aa8359 Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Sun, 5 Oct 2025 16:01:11 +0530 Subject: [PATCH 1/2] HBASE-29611: With FILE based SFT, the list of HFiles we maintain in .filelist is not getting updated for read replica Link to JIRA: https://issues.apache.org/jira/browse/HBASE-29611 Description: Steps to Repro (For detailed steps, check JIRA): - Create two clusters on the same storage location. - Create table on active, then refresh meta on the read replica to get the table meta data updated. - Add some rows and flush on the active cluster, do refresh_hfiles on the read replica and scan table. - If you now again add the rows in the table on active and do refresh_hfiles then the rows added are not visible in the read replica. Cause: The refresh store file is a two step process: 1. Load the existing store file from the .filelist (choose the file with higher timestamp for loading) 2. refresh store file internals (clean up old/compacted files, replace store file in .filelist) In the current scenario, what is happening is that for the first time read-replica is loading the list of Hfiles from the file in .filelist created by active cluster but then it is creating the new file with greater timestamp. Now we have two files in .filelist. On the subsequent flush from active the file in .filelist created by the active gets updated but the file created by read-replica is not. While loading in the refresh_hfiles as we take the file with higher timestamp the file created by read-replica for the first time gets loaded which does not have an updated list of hfiles. Fix: As we just wanted the file from active to be loaded anytime we perform refresh store files, we must not create a new file in the .filelist from the read-replica, in this way we will stop the timestamp mismatch. NOTE: Also we don't want to initialize the tracker file (StoreFileListFile.java:load()) from read-replica as we are not writing it hence we have added check for read only property in StoreFileTrackerBase.java:load() --- .../StoreFileTrackerBase.java | 10 ++- .../DummyStoreFileTrackerForReadOnlyMode.java | 69 ++++++++++++++ .../TestStoreFileTrackerBaseReadOnlyMode.java | 90 +++++++++++++++++++ 3 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DummyStoreFileTrackerForReadOnlyMode.java create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerBaseReadOnlyMode.java 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 779a114af594..be67c154c2f6 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 @@ -32,6 +32,7 @@ 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.TableName; import org.apache.hadoop.hbase.backup.HFileArchiver; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; @@ -85,7 +86,9 @@ protected StoreFileTrackerBase(Configuration conf, boolean isPrimaryReplica, Sto @Override public final List load() throws IOException { - return doLoadStoreFiles(!isPrimaryReplica); + return doLoadStoreFiles( + !isPrimaryReplica || conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, + HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT)); } @Override @@ -98,7 +101,10 @@ public final void add(Collection newFiles) throws IOException { @Override public final void replace(Collection compactedFiles, Collection newFiles) throws IOException { - if (isPrimaryReplica) { + if ( + isPrimaryReplica && !conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, + HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT) + ) { doAddCompactionResults(compactedFiles, newFiles); } } 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 new file mode 100644 index 000000000000..b3d04abac77f --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/DummyStoreFileTrackerForReadOnlyMode.java @@ -0,0 +1,69 @@ +/* + * 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.storefiletracker; + +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.regionserver.StoreFileInfo; + +public class DummyStoreFileTrackerForReadOnlyMode extends StoreFileTrackerBase { + private boolean readOnlyUsed = false; + private boolean compactionExecuted = false; + + public DummyStoreFileTrackerForReadOnlyMode(Configuration conf, boolean isPrimaryReplica) { + super(conf, isPrimaryReplica, null); + } + + @Override + protected void doAddCompactionResults(Collection compactedFiles, + Collection newFiles) { + compactionExecuted = true; + } + + @Override + protected void doSetStoreFiles(Collection files) throws IOException { + + } + + @Override + protected List doLoadStoreFiles(boolean readOnly) { + readOnlyUsed = readOnly; + return Collections.emptyList(); + } + + @Override + protected void doAddNewStoreFiles(Collection newFiles) throws IOException { + + } + + boolean wasReadOnlyLoad() { + return readOnlyUsed; + } + + boolean wasCompactionExecuted() { + return compactionExecuted; + } + + @Override + public boolean requireWritingToTmpDirFirst() { + return false; + } +} 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 new file mode 100644 index 000000000000..f4d1757a4e80 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerBaseReadOnlyMode.java @@ -0,0 +1,90 @@ +/* + * 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.storefiletracker; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.util.Collections; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.TestRefreshHFilesBase; +import org.apache.hadoop.hbase.master.procedure.TestRefreshHFilesProcedureWithReadOnlyConf; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.After; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ RegionServerTests.class, SmallTests.class }) +public class TestStoreFileTrackerBaseReadOnlyMode extends TestRefreshHFilesBase { + private DummyStoreFileTrackerForReadOnlyMode tracker; + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestRefreshHFilesProcedureWithReadOnlyConf.class); + + @Before + public void setup() throws Exception { + // When true is passed only setup for readonly property is done. + // The initial ReadOnly property will be false for table creation + baseSetup(true); + } + + @After + public void tearDown() throws Exception { + baseTearDown(); + } + + @Test + public void testLoadReadOnlyWhenGlobalReadOnlyEnabled() throws Exception { + try { + setReadOnlyMode(true); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + tracker.load(); + assertTrue("Tracker should be in read-only mode", tracker.wasReadOnlyLoad()); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + setReadOnlyMode(false); + } + } + + @Test + public void testReplaceSkippedWhenGlobalReadOnlyEnabled() throws Exception { + try { + setReadOnlyMode(true); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + tracker.replace(Collections.emptyList(), Collections.emptyList()); + assertFalse("Compaction should not be executed in readonly mode", + tracker.wasCompactionExecuted()); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + setReadOnlyMode(false); + } + } + + @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()); + } +} From 12ee014ea1f9e9abf3a403250c2aebb893b1f0c5 Mon Sep 17 00:00:00 2001 From: Anuj Sharma Date: Thu, 23 Oct 2025 15:23:42 +0530 Subject: [PATCH 2/2] Make read only cluster to behave like secondary replica --- .../StoreFileTrackerBase.java | 23 ++++---- .../DummyStoreFileTrackerForReadOnlyMode.java | 14 ++++- .../TestStoreFileTrackerBaseReadOnlyMode.java | 56 +++++++++++++++++++ 3 files changed, 80 insertions(+), 13 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 be67c154c2f6..bc24ad579444 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 @@ -86,14 +86,12 @@ protected StoreFileTrackerBase(Configuration conf, boolean isPrimaryReplica, Sto @Override public final List load() throws IOException { - return doLoadStoreFiles( - !isPrimaryReplica || conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, - HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT)); + return doLoadStoreFiles(!isPrimaryReplica || isReadOnlyEnabled()); } @Override public final void add(Collection newFiles) throws IOException { - if (isPrimaryReplica) { + if (isPrimaryReplica && !isReadOnlyEnabled()) { doAddNewStoreFiles(newFiles); } } @@ -101,17 +99,14 @@ public final void add(Collection newFiles) throws IOException { @Override public final void replace(Collection compactedFiles, Collection newFiles) throws IOException { - if ( - isPrimaryReplica && !conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, - HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT) - ) { + if (isPrimaryReplica && !isReadOnlyEnabled()) { doAddCompactionResults(compactedFiles, newFiles); } } @Override public final void set(List files) throws IOException { - if (isPrimaryReplica) { + if (isPrimaryReplica && !isReadOnlyEnabled()) { doSetStoreFiles(files); } } @@ -146,8 +141,9 @@ private HFileContext createFileContext(Compression.Algorithm compression, @Override public final StoreFileWriter createWriter(CreateStoreFileWriterParams params) throws IOException { - if (!isPrimaryReplica) { - throw new IllegalStateException("Should not call create writer on secondary replicas"); + if (!isPrimaryReplica || isReadOnlyEnabled()) { + throw new IllegalStateException( + "Should not call create writer on secondary replicas or in read only mode"); } // creating new cache config for each new writer final CacheConfig cacheConf = ctx.getCacheConf(); @@ -391,6 +387,11 @@ 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/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 b3d04abac77f..5f8268b3ac7c 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 @@ -27,6 +27,8 @@ public class DummyStoreFileTrackerForReadOnlyMode extends StoreFileTrackerBase { 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); @@ -40,7 +42,7 @@ protected void doAddCompactionResults(Collection compactedFiles, @Override protected void doSetStoreFiles(Collection files) throws IOException { - + setExecuted = true; } @Override @@ -51,7 +53,7 @@ protected List doLoadStoreFiles(boolean readOnly) { @Override protected void doAddNewStoreFiles(Collection newFiles) throws IOException { - + addExecuted = true; } boolean wasReadOnlyLoad() { @@ -62,6 +64,14 @@ boolean wasCompactionExecuted() { return compactionExecuted; } + boolean wasAddExecuted() { + return addExecuted; + } + + boolean wasSetExecuted() { + return setExecuted; + } + @Override public boolean requireWritingToTmpDirFirst() { return false; 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 f4d1757a4e80..8a1cee55832a 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 @@ -24,6 +24,7 @@ import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.TestRefreshHFilesBase; import org.apache.hadoop.hbase.master.procedure.TestRefreshHFilesProcedureWithReadOnlyConf; +import org.apache.hadoop.hbase.regionserver.CreateStoreFileWriterParams; import org.apache.hadoop.hbase.testclassification.RegionServerTests; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.junit.After; @@ -87,4 +88,59 @@ public void testReplaceExecutedWhenWritable() throws Exception { tracker.replace(Collections.emptyList(), Collections.emptyList()); assertTrue("Compaction should run when not readonly", tracker.wasCompactionExecuted()); } + + @Test + public void testAddSkippedWhenGlobalReadOnlyEnabled() throws Exception { + try { + setReadOnlyMode(true); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + tracker.add(Collections.emptyList()); + assertFalse("Add should not be executed in readonly mode", tracker.wasAddExecuted()); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + setReadOnlyMode(false); + } + } + + @Test + public void testAddExecutedWhenWritable() throws Exception { + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + tracker.add(Collections.emptyList()); + assertTrue("Add should run when not readonly", tracker.wasAddExecuted()); + } + + @Test + public void testSetSkippedWhenGlobalReadOnlyEnabled() throws Exception { + try { + setReadOnlyMode(true); + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + tracker.set(Collections.emptyList()); + assertFalse("Set should not be executed in readonly mode", tracker.wasSetExecuted()); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + setReadOnlyMode(false); + } + } + + @Test + public void testSetExecutedWhenWritable() throws Exception { + tracker = new DummyStoreFileTrackerForReadOnlyMode(conf, true); + tracker.set(Collections.emptyList()); + assertTrue("Set should run when not readonly", tracker.wasSetExecuted()); + } + + @Test(expected = IllegalStateException.class) + public void testCreateWriterThrowExceptionWhenGlobalReadOnlyEnabled() 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); + } finally { + setReadOnlyMode(false); + } + } }