Copilot commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r2988964975
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -183,6 +184,62 @@ public void testBlockStoragePolicy() throws Exception {
TEST_UTIL.shutdownMiniCluster();
}
}
+ @Test
+ public void testMobStoreStoragePolicy() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ Configuration conf = TEST_UTIL.getConfiguration();
+ TEST_UTIL.startMiniCluster();
+ Table table = TEST_UTIL.createTable(TABLE_NAME, FAMILIES);
+ assertEquals("Should start with empty table", 0,
TEST_UTIL.countRows(table));
+ HRegionFileSystem regionFs = getHRegionFS(TEST_UTIL.getConnection(),
table, conf);
+ try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
+ ColumnFamilyDescriptorBuilder cfdA =
ColumnFamilyDescriptorBuilder.newBuilder(FAMILIES[0]);
+ cfdA.setValue(HStore.BLOCK_STORAGE_POLICY_KEY, "ONE_SSD");
+ cfdA.setMobEnabled(true);
+ cfdA.setMobThreshold(2L);
+ admin.modifyColumnFamily(TABLE_NAME, cfdA.build());
+ while (
+
TEST_UTIL.getMiniHBaseCluster().getMaster().getAssignmentManager().getRegionStates()
+ .hasRegionsInTransition()
+ ) {
Review Comment:
The wait loop calls
`getAssignmentManager().getRegionStates().hasRegionsInTransition()`, but
`RegionStates` does not define `hasRegionsInTransition()` in this codebase, so
this won't compile. Use `getAssignmentManager().hasRegionsInTransition()` (as
in `testBlockStoragePolicy`) or another existing API for RIT checks.
```suggestion
while
(TEST_UTIL.getMiniHBaseCluster().getMaster().getAssignmentManager()
.hasRegionsInTransition()) {
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HMobStore.java:
##########
@@ -110,6 +111,7 @@ public HMobStore(final HRegion region, final
ColumnFamilyDescriptor family,
this.homePath = MobUtils.getMobHome(conf);
this.mobFamilyPath =
MobUtils.getMobFamilyPath(conf, this.getTableName(),
getColumnFamilyName());
+ CommonFSUtils.setStoragePolicy(this.getFileSystem(), mobFamilyPath,
this.policyName);
Review Comment:
`CommonFSUtils.setStoragePolicy` is invoked on `mobFamilyPath` during store
construction, but the MOB family directory may not exist yet (e.g.,
`DefaultMobStoreFlusher` creates `targetPath` with `mkdirs` if it doesn't
exist). If the path is missing, `setStoragePolicy` will log a warning and the
policy won't be applied, so MOB files created later won't inherit it. Ensure
the MOB family directory exists before setting the policy (or set the policy at
the point where the directory is created).
```suggestion
FileSystem fs = this.getFileSystem();
if (!fs.exists(mobFamilyPath)) {
fs.mkdirs(mobFamilyPath);
}
CommonFSUtils.setStoragePolicy(fs, mobFamilyPath, this.policyName);
```
--
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]
