HBASE-28177 Fix mobStore directory do not set storage policy by xieyupei · Pull Request #6385 · apache/hbase

xieyupei · 2024-10-23T07:24:13Z

No description provided.

NihalJain · 2024-10-23T07:38:03Z

 private AtomicLong majorCompactedCellsSize = new AtomicLong();

 private final StoreContext storeContext;
+ protected String policyName;


Hi @xieyupei do we really need this? why not just do String policyName = family.getStoragePolicy(); even in HMobStore.java

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();
"

Yes we should do that right?

yes, I think so

NihalJain · 2024-10-23T07:44:13Z

+ 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");


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

ok, i agree. what about the "hbase.hmobstore.block.storage.policy" default value? equals hstore or just none?

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.

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

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.

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?

This PR has been around for a while, may not have noticed, remind. @Apache9

Apache-HBase · 2024-10-23T08:10:10Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 18s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ master Compile Tests _
+1 💚	mvninstall	3m 0s		master passed
+1 💚	compile	2m 54s		master passed
+1 💚	checkstyle	0m 37s		master passed
-1 ❌	spotbugs	1m 32s	/branch-spotbugs-hbase-server-warnings.html	hbase-server in master has 1 extant spotbugs warnings.
+1 💚	spotless	0m 45s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+1 💚	mvninstall	2m 51s		the patch passed
+1 💚	compile	2m 54s		the patch passed
+1 💚	javac	2m 54s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 37s		the patch passed
+1 💚	spotbugs	1m 40s		the patch passed
+1 💚	hadoopcheck	10m 5s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
-1 ❌	spotless	0m 37s		patch has 57 errors when running spotless:check, run spotless:apply to fix.
			_ Other Tests _
+1 💚	asflicense	0m 12s		The patch does not generate ASF License warnings.
		34m 30s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6385
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 6c87bd5544ba 5.4.0-192-generic #212-Ubuntu SMP Fri Jul 5 09:47:39 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `2834c1c`
Default Java	Eclipse Adoptium-17.0.11+9
spotless	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/artifact/yetus-general-check/output/patch-spotless.txt
Max. process+thread count	83 (vs. ulimit of 30000)
modules	C: hbase-server U: hbase-server
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2024-10-23T11:25:20Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 26s		Docker mode activated.
-0 ⚠️	yetus	0m 2s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ master Compile Tests _
+1 💚	mvninstall	2m 56s		master passed
+1 💚	compile	0m 51s		master passed
+1 💚	javadoc	0m 26s		master passed
+1 💚	shadedjars	5m 25s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	2m 48s		the patch passed
+1 💚	compile	0m 54s		the patch passed
+1 💚	javac	0m 54s		the patch passed
+1 💚	javadoc	0m 25s		the patch passed
+1 💚	shadedjars	5m 22s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	205m 55s		hbase-server in the patch passed.
		229m 39s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6385
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 01d276056ddf 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `2834c1c`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/testReport/
Max. process+thread count	5127 (vs. ulimit of 30000)
modules	C: hbase-server U: hbase-server
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

NihalJain · 2024-10-28T11:23:45Z

- 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())


is there any change in logic here? Seems same as before just that we are using lambdas now?

yes, no logic change. just to simplify assigning values to "this.policyName"

The lambda does not help too much here I'd say.
Let's keep the old way?
String pName = family.getStoragePolicy() != null ? family.getStoragePolicy() : conf.get(BLOCK_STORAGE_POLICY_KEY, DEFAULT_BLOCK_STORAGE_POLICY); this.policyName = pName.trim();

ok, I will do it

NihalJain · 2026-03-25T15:12:51Z

Ah missed following up here, sorry for the delay @xieyupei

Copilot

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 HStore for 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.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

xieyupei · 2026-03-27T07:04:11Z

Ah missed following up here, sorry for the delay @xieyupei
Thanks for your attention, I also forget this PR for long time. @NihalJain

xieyupei · 2026-03-30T07:21:42Z

@NihalJain Could you please help me what to do next?Is it means this PR is expired?

NihalJain · 2026-03-30T11:26:05Z

Hi @xieyupei lgtm, will wait for more eyes just in case, else merge by tomorrow EOD

xieyupeiand others added 4 commits July 23, 2023 15:43

HBASE-27717 Add rsgroup name for dead region servers on master UI
82847f3

fix code to make it compliant
f96839e

Merge branch 'master' ofhttps://github.com/apache/hbase
497b2bf

HBASE-28177 Fix mobStore directory do not set storage policy
2834c1c

NihalJain reviewed Oct 23, 2024
View reviewed changes

NihalJain reviewed Oct 28, 2024
View reviewed changes

Merge branch 'master' into master
c7aa06b

NihalJain requested a review from Copilot March 25, 2026 15:12

Copilotstarted reviewing on behalf of NihalJain March 25, 2026 15:12 View session

CopilotAI reviewed Mar 25, 2026
View reviewed changes

Comment threadhbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java Outdated
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HMobStore.java Outdated

xieyupeiand others added 2 commits March 27, 2026 14:56

Apply suggestion from @Copilot
ea51cef
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestion from @Copilot
5eb344b
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

NihalJain approved these changes Mar 30, 2026
View reviewed changes

NihalJain requested a review from guluo2016 March 30, 2026 11:25

xieyupei added 2 commits March 31, 2026 11:05

Merge branch 'master' ofhttps://github.com/xieyupei/hbaseinto mymaster
052dba5

Simplify the assignment of value to the policyName
0146b85

xieyupei requested review from Apache9 and NihalJain April 1, 2026 02:54

Uh oh!

Conversation

xieyupei commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Apache-HBase commented Oct 23, 2024

Uh oh!

Apache-HBase commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihalJain commented Mar 25, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

xieyupei commented Mar 27, 2026

Uh oh!

xieyupei commented Mar 30, 2026

Uh oh!

NihalJain commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants