You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "xieyupei (via GitHub)" <gi...@apache.org> on 2024/10/23 07:24:16 UTC
[PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
xieyupei opened a new pull request, #6385:
URL: https://github.com/apache/hbase/pull/6385
(no comment)
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1820411627
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -264,11 +265,10 @@ protected HStore(final HRegion region, final ColumnFamilyDescriptor family,
region.getRegionFileSystem().createStoreDir(family.getNameAsString());
// set block storage policy for store directory
- String policyName = family.getStoragePolicy();
- if (null == policyName) {
- 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())
Review Comment:
yes, no logic change. just to simplify assigning values to "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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1814523918
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review Comment:
ok, i agree. what about the "hbase.hmobstore.block.storage.policy" default value? equals hstore or just none?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on PR #6385:
URL: https://github.com/apache/hbase/pull/6385#issuecomment-4127397794
Ah missed following up here, sorry for the delay @xieyupei
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on PR #6385:
URL: https://github.com/apache/hbase/pull/6385#issuecomment-4152832337
@NihalJain Could you please help me what to do next?Is it means this PR is expired?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "guluo2016 (via GitHub)" <gi...@apache.org>.
guluo2016 commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1814533478
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review 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.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r3019478145
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -264,11 +265,10 @@ protected HStore(final HRegion region, final ColumnFamilyDescriptor family,
region.getRegionFileSystem().createStoreDir(family.getNameAsString());
// set block storage policy for store directory
- String policyName = family.getStoragePolicy();
- if (null == policyName) {
- 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())
Review Comment:
ok, I will do it
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1812084701
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review 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
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on PR #6385:
URL: https://github.com/apache/hbase/pull/6385#issuecomment-4154285901
Hi @xieyupei lgtm, will wait for more eyes just in case, else merge by tomorrow EOD
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1818881617
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review 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?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1818890357
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -264,11 +265,10 @@ protected HStore(final HRegion region, final ColumnFamilyDescriptor family,
region.getRegionFileSystem().createStoreDir(family.getNameAsString());
// set block storage policy for store directory
- String policyName = family.getStoragePolicy();
- if (null == policyName) {
- 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())
Review Comment:
is there any change in logic here? Seems same as before just that we are using lambdas now?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #6385:
URL: https://github.com/apache/hbase/pull/6385#issuecomment-2431240161
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 18s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any anti-patterns. |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 0s | | master passed |
| +1 :green_heart: | compile | 2m 54s | | master passed |
| +1 :green_heart: | checkstyle | 0m 37s | | master passed |
| -1 :x: | spotbugs | 1m 32s | [/branch-spotbugs-hbase-server-warnings.html](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6385/1/artifact/yetus-general-check/output/branch-spotbugs-hbase-server-warnings.html) | hbase-server in master has 1 extant spotbugs warnings. |
| +1 :green_heart: | spotless | 0m 45s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 51s | | the patch passed |
| +1 :green_heart: | compile | 2m 54s | | the patch passed |
| +1 :green_heart: | javac | 2m 54s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 37s | | the patch passed |
| +1 :green_heart: | spotbugs | 1m 40s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 10m 5s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.0. |
| -1 :x: | spotless | 0m 37s | | patch has 57 errors when running spotless:check, run spotless:apply to fix. |
|||| _ Other Tests _ |
| +1 :green_heart: | 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 | https://github.com/apache/hbase/pull/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 / 2834c1cfa07defa9fc9c2a49ccbb62774ee91c3c |
| 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.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1820413937
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -212,6 +212,7 @@ public class HStore
private AtomicLong majorCompactedCellsSize = new AtomicLong();
private final StoreContext storeContext;
+ protected String policyName;
Review Comment:
yes, I think so
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1812073044
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -212,6 +212,7 @@ public class HStore
private AtomicLong majorCompactedCellsSize = new AtomicLong();
private final StoreContext storeContext;
+ protected String policyName;
Review Comment:
Hi @xieyupei do we really need this? why not just do `String policyName = family.getStoragePolicy();` even in `HMobStore.java`
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1855915424
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review Comment:
This PR has been around for a while, may not have noticed, remind. @Apache9
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #6385:
URL: https://github.com/apache/hbase/pull/6385#issuecomment-2431798141
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 26s | | Docker mode activated. |
| -0 :warning: | 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 :green_heart: | mvninstall | 2m 56s | | master passed |
| +1 :green_heart: | compile | 0m 51s | | master passed |
| +1 :green_heart: | javadoc | 0m 26s | | master passed |
| +1 :green_heart: | shadedjars | 5m 25s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 48s | | the patch passed |
| +1 :green_heart: | compile | 0m 54s | | the patch passed |
| +1 :green_heart: | javac | 0m 54s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 25s | | the patch passed |
| +1 :green_heart: | shadedjars | 5m 22s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | 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 | https://github.com/apache/hbase/pull/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 / 2834c1cfa07defa9fc9c2a49ccbb62774ee91c3c |
| 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.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1814536557
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -212,6 +212,7 @@ public class HStore
private AtomicLong majorCompactedCellsSize = new AtomicLong();
private final StoreContext storeContext;
+ protected String policyName;
Review 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();
"
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on PR #6385:
URL: https://github.com/apache/hbase/pull/6385#issuecomment-4140625213
> Ah missed following up here, sorry for the delay @xieyupei
Thanks for your attention, I also forget this PR for long time. @NihalJain
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "Apache9 (via GitHub)" <gi...@apache.org>.
Apache9 commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r3009236701
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -264,11 +265,10 @@ protected HStore(final HRegion region, final ColumnFamilyDescriptor family,
region.getRegionFileSystem().createStoreDir(family.getNameAsString());
// set block storage policy for store directory
- String policyName = family.getStoragePolicy();
- if (null == policyName) {
- 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())
Review Comment:
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();
```
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "guluo2016 (via GitHub)" <gi...@apache.org>.
guluo2016 commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1818855950
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review 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.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1818888562
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java:
##########
@@ -212,6 +212,7 @@ public class HStore
private AtomicLong majorCompactedCellsSize = new AtomicLong();
private final StoreContext storeContext;
+ protected String policyName;
Review Comment:
Yes we should do that right?
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28177 Fix mobStore directory do not set storage policy [hbase]
Posted by "xieyupei (via GitHub)" <gi...@apache.org>.
xieyupei commented on code in PR #6385:
URL: https://github.com/apache/hbase/pull/6385#discussion_r1818312283
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java:
##########
@@ -184,6 +185,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");
Review 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
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org