anmolnar commented on code in PR #7330:
URL: https://github.com/apache/hbase/pull/7330#discussion_r2389273882
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegionFactory.java:
##########
@@ -54,6 +56,7 @@ public final class MasterRegionFactory {
public static final String USE_HSYNC_KEY =
"hbase.master.store.region.wal.hsync";
+ // Default master data dir
public static final String MASTER_STORE_DIR = "MasterData";
Review Comment:
Can we make this private to make sure nobody is using it outside of this
class?
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/region/TestMasterRegionInitialize.java:
##########
@@ -114,4 +118,25 @@ public void testInitializingCleanup() throws IOException {
// but the data should have been cleaned up
assertTrue(region.get(new Get(row)).isEmpty());
}
+
+ @Test
+ public void testMasterRegionDirSuffix() throws Exception {
+ HBaseTestingUtil testUtil = new HBaseTestingUtil();
+ Configuration conf = testUtil.getConfiguration();
+ conf.set(HConstants.HBASE_META_TABLE_SUFFIX, "replica1");
+
+ try {
+ testUtil.startMiniCluster(1);
+ FileSystem fs = CommonFSUtils.getRootDirFileSystem(conf);
+ Path rootDir = CommonFSUtils.getRootDir(conf);
+
+ assertTrue("Master region directory should use the suffix",
+ fs.exists(new Path(rootDir, "MasterData_replica1")));
+ assertFalse("Default master region directory should not exist when
suffix is used",
+ fs.exists(new Path(rootDir, "MasterData")));
Review Comment:
Same here.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/region/TestMasterRegionInitialize.java:
##########
@@ -114,4 +118,25 @@ public void testInitializingCleanup() throws IOException {
// but the data should have been cleaned up
assertTrue(region.get(new Get(row)).isEmpty());
}
+
+ @Test
+ public void testMasterRegionDirSuffix() throws Exception {
+ HBaseTestingUtil testUtil = new HBaseTestingUtil();
+ Configuration conf = testUtil.getConfiguration();
+ conf.set(HConstants.HBASE_META_TABLE_SUFFIX, "replica1");
+
+ try {
+ testUtil.startMiniCluster(1);
+ FileSystem fs = CommonFSUtils.getRootDirFileSystem(conf);
+ Path rootDir = CommonFSUtils.getRootDir(conf);
+
+ assertTrue("Master region directory should use the suffix",
+ fs.exists(new Path(rootDir, "MasterData_replica1")));
Review Comment:
I don't think this is needed. Tests are good to have hardcoded values in
order to detect anomalies in the code which happened after the test was
introduced.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/procedure2/store/region/HFileProcedurePrettyPrinter.java:
##########
@@ -85,7 +85,7 @@ protected void addOptions() {
private void addAllHFiles() throws IOException {
Path masterProcDir =
- new Path(CommonFSUtils.getRootDir(conf),
MasterRegionFactory.MASTER_STORE_DIR);
+ new Path(CommonFSUtils.getRootDir(conf),
MasterRegionFactory.getMasterRegionDirName(conf));
Review Comment:
Instead of calling the setter from here, add a new read-only property which
will return the correct region dir name. Essentially making the property
immutable.
--
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]
