HBASE-28177 Fix mobStore directory do not set storage policy#6385
HBASE-28177 Fix mobStore directory do not set storage policy#6385xieyupei wants to merge 4 commits intoapache:masterfrom
Conversation
| private AtomicLong majorCompactedCellsSize = new AtomicLong(); | ||
|
|
||
| private final StoreContext storeContext; | ||
| protected String policyName; |
There was a problem hiding this comment.
Hi @xieyupei do we really need this? why not just do String policyName = family.getStoragePolicy(); even in HMobStore.java
There was a problem hiding this comment.
Before this key "hbase.hmobstore.block.storage.policy", i think if we don't have this, we should write the same code in HMobStore.
"Optional.ofNullable(family.getStoragePolicy())
.orElseGet(() -> conf.get(BLOCK_STORAGE_POLICY_KEY, DEFAULT_BLOCK_STORAGE_POLICY))
.trim();
"
There was a problem hiding this comment.
Yes we should do that right?
| 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"); |
There was a problem hiding this comment.
Should we add a new key: "hbase.hmobstore.block.storage.policy"
I am not sure if this JIRA is a bug or a new feature. But teh change will definitely change behaviour for exisdting users once this patch is merged
There was a problem hiding this comment.
ok, i agree. what about the "hbase.hmobstore.block.storage.policy" default value? equals hstore or just none?
There was a problem hiding this comment.
I don't think we need to add this new key, because when we creat a table, we set the STORAGE-POLICY configuration or ues the default policy (HStore.BLOCK_STORAGE_POLICY_KEY), which means that all data under this column family should follow this policy.
So, the storage strategy for MOB should be consistent with the column family.
There was a problem hiding this comment.
We discovered this issue when we found incorrect storage capacity in our cluster. We believe this is a bug, so I fix it like this pr, but I'm not sure how this is determined in our community. @NihalJain @guluo2016
There was a problem hiding this comment.
We believe this is a bug, so I fix it like this pr
I agree with you.
but I'm not sure how this is determined in our community.
In my personal opinion, the storage strategy for MOB should be consistent with the column family.
Perhaps we can discuss it with Nihal and others together.
There was a problem hiding this comment.
I am fine with consolidating with mobstore as that is the right way. My only concern is we may unknowingly start moving data around if this silently lands with a new version upgrade. May be we are okay to just add to release note, since that would be a behavioral change (due to bug).
Hey @Apache9 WDYT?
There was a problem hiding this comment.
This PR has been around for a while, may not have noticed, remind. @Apache9
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| policyName = this.conf.get(BLOCK_STORAGE_POLICY_KEY, DEFAULT_BLOCK_STORAGE_POLICY); | ||
| } | ||
| region.getRegionFileSystem().setStoragePolicy(family.getNameAsString(), policyName.trim()); | ||
| this.policyName = Optional.ofNullable(family.getStoragePolicy()) |
There was a problem hiding this comment.
is there any change in logic here? Seems same as before just that we are using lambdas now?
There was a problem hiding this comment.
yes, no logic change. just to simplify assigning values to "this.policyName"
|
Ah missed following up here, sorry for the delay @xieyupei |
There was a problem hiding this comment.
Pull request overview
This PR (HBASE-28177) aims to ensure MOB store directories/files correctly inherit the column family storage policy, aligning MOB behavior with normal store files.
Changes:
- Expose the resolved CF storage policy in
HStorefor reuse by subclasses. - Apply the resolved storage policy to the MOB family directory in
HMobStore. - Add a regionserver filesystem test validating storage policy inheritance for MOB store files.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java | Persist resolved CF storage policy for reuse (e.g., by MOB store). |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HMobStore.java | Attempt to apply CF storage policy to MOB family directory. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java | Add test coverage for MOB store storage policy behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HMobStore.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
No description provided.