HBASE-29668 Add row cache framework by EungsopYoo · Pull Request #7398 · apache/hbase

EungsopYoo · 2025-10-19T23:35:38Z

No description provided.

wchevreuil · 2025-10-22T08:53:43Z

I see this PR got an unrelated commit. Do you want me to rebase this branch on top of master, @EungsopYoo? You can then just force push your initial commit to your remote branch.

EungsopYoo · 2025-10-22T09:34:04Z

@wchevreuil
I’ve rebased it myself. Do you mean like this?

EungsopYoo · 2025-11-24T03:46:52Z

@wchevreuil
Could you continue reviewing this PR?

wchevreuil

Thanks for submitting this PR @EungsopYoo and sorry for the long delayed response. Let's resume the discussions on this. Please refer to my comments below:

I think we should avoid putting row cache related logic on RSRpcServices, as RSRpcServices is mainly a proxy between RS/Region logic and the RPC layer.

The row cache would be a core component of RegionServer, so in that sense, it should be created and kept at HRegionServer, just like we do for BlockCache.

Since row cache would be used on region related operations, all access logic to it should be implemented at HRegion class. HRegionServer should expose its row cache to HRegion instances via the RegionServerServices (as it does with BlockCache, for instance).

wchevreuil · 2026-01-14T11:11:46Z

+package org.apache.hadoop.hbase.regionserver;
+
+@org.apache.yetus.audience.InterfaceAudience.Private
+public class RowCacheImpl implements RowCache {


nit: since we are not really doing anything here, can we remove this impl from this PR and focus only on the framework itself?

EungsopYoo · 2026-01-14T13:38:54Z

@wchevreuil
Understood. I’m currently on vacation, so I’ll review your comments next week.

wchevreuil · 2026-01-19T11:28:48Z

@wchevreuil Understood. I’m currently on vacation, so I’ll review your comments next week.

No problem. Enjoy your time off!

EungsopYoo · 2026-01-22T23:18:11Z

I have a lot of backlog, so I think I’ll be able to resume in about one to two weeks.

EungsopYoo · 2026-02-04T04:56:47Z

@wchevreuil
The apache:HBASE-29585 branch was created a few months ago. So wouldn’t it be better to sync it with the latest changes before we start modifying it? What’s the best way to proceed?

Should I open a separate PR for the update, or could you take care of it directly?

Apache-HBase · 2026-02-04T05:35:00Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 12s		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.
			_ HBASE-29585 Compile Tests _
+0 🆗	mvndep	0m 10s		Maven dependency ordering for branch
+1 💚	mvninstall	2m 40s		HBASE-29585 passed
+1 💚	compile	4m 4s		HBASE-29585 passed
+1 💚	checkstyle	0m 55s		HBASE-29585 passed
+1 💚	spotbugs	2m 15s		HBASE-29585 passed
+1 💚	spotless	0m 41s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 19s		the patch passed
+1 💚	compile	4m 0s		the patch passed
+1 💚	javac	4m 0s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 8s	/buildtool-patch-checkstyle-hbase-server.txt	The patch fails to run checkstyle in hbase-server
-0 ⚠️	rubocop	0m 4s	/results-rubocop.txt	The patch generated 2 new + 411 unchanged - 0 fixed = 413 total (was 411)
+1 💚	spotbugs	2m 27s		the patch passed
+1 💚	hadoopcheck	8m 48s		Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
+1 💚	spotless	0m 34s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 30s		The patch does not generate ASF License warnings.
		36m 41s

Subsystem	Report/Notes
Docker	ClientAPI=1.53 ServerAPI=1.53 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7398/14/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#7398
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless rubocop
uname	Linux bacbee5c8d09 6.14.0-1018-aws #18~24.04.1-Ubuntu SMP Mon Nov 24 19:46:27 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	HBASE-29585 / `927b2e6`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	85 (vs. ulimit of 30000)
modules	C: hbase-common hbase-client hbase-server hbase-shell U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7398/14/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3 rubocop=1.37.1
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2026-02-04T09:33:33Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 36s		Docker mode activated.
-0 ⚠️	yetus	0m 5s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ HBASE-29585 Compile Tests _
+0 🆗	mvndep	0m 21s		Maven dependency ordering for branch
+1 💚	mvninstall	4m 27s		HBASE-29585 passed
+1 💚	compile	1m 54s		HBASE-29585 passed
+1 💚	javadoc	1m 12s		HBASE-29585 passed
+1 💚	shadedjars	6m 7s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 14s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 58s		the patch passed
+1 💚	compile	1m 55s		the patch passed
+1 💚	javac	1m 55s		the patch passed
+1 💚	javadoc	1m 11s		the patch passed
+1 💚	shadedjars	6m 1s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	2m 25s		hbase-common in the patch passed.
+1 💚	unit	1m 31s		hbase-client in the patch passed.
+1 💚	unit	230m 24s		hbase-server in the patch passed.
+1 💚	unit	7m 25s		hbase-shell in the patch passed.
		275m 3s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7398/14/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#7398
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 51b405efab84 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	HBASE-29585 / `927b2e6`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7398/14/testReport/
Max. process+thread count	4435 (vs. ulimit of 30000)
modules	C: hbase-common hbase-client hbase-server hbase-shell U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7398/14/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.

EungsopYoo · 2026-02-09T08:02:06Z

Thanks for submitting this PR @EungsopYoo and sorry for the long delayed response. Let's resume the discussions on this. Please refer to my comments below:
I think we should avoid putting row cache related logic on RSRpcServices, as RSRpcServices is mainly a proxy between RS/Region logic and the RPC layer.
The row cache would be a core component of RegionServer, so in that sense, it should be created and kept at HRegionServer, just like we do for BlockCache.
Since row cache would be used on region related operations, all access logic to it should be implemented at HRegion class. HRegionServer should expose its row cache to HRegion instances via the RegionServerServices (as it does with BlockCache, for instance).

4707c74

wchevreuil · 2026-02-10T13:59:46Z


+ RegionScannerImpl getScannerWithResults(Get get, Scan scan, List<Cell> results)
+ throws IOException {
+ return getRowCacheService().getScanner(this, get, scan, results);


We'll always go for the row cache? What if it's disabled?

This is determined in RowCacheService.getScanner(). If the row cache is disabled, the system falls back to the traditional read path.

wchevreuil · 2026-02-10T14:29:57Z

+ RegionScannerImpl getScanner(HRegion region, Get get, Scan scan, List<Cell> results)
+ throws IOException {
+ if (!canCacheRow(get, region)) {
+ return getScannerInternal(region, scan, results);
+ }
+
+ RowCacheKey key = new RowCacheKey(region, get.getRow());
+
+ // Try get from row cache
+ if (tryGetFromCache(region, key, get, results)) {
+ // Cache is hit, and then no scanner is created
+ return null;
+ }
+
+ RegionScannerImpl scanner = getScannerInternal(region, scan, results);
+ populateCache(region, results, key);
+ return scanner;
+ }


I think RowCacheService shouldn't bother about RegionScannerImpl, but rather only the cache itself, so it should only check for the rowkey in the cache and populate the results accordingly. It's then up for the HRegion to create the scanner when no results are found in the row cache. So we could make this method void, or return List with the found results, and get rid of the scanner creation logic. These would be done in HRegion.getScannerResults.

wchevreuil · 2026-02-10T14:39:32Z

+ RegionScannerImpl scanner = region.getScanner(scan);
+ scanner.next(results);
+ return scanner;


Should move to HRegion (see my previous comment above).

wchevreuil · 2026-02-10T14:51:53Z

+ return getScannerInternal(region, scan, results);
+ }
+
+ RowCacheKey key = new RowCacheKey(region, get.getRow());


Could we use "namespace + tableame + rowkey" as the UID of a row in the row cache? We could then only pass the TableDescriptor, rather than the HRegion as this method parameter.

If we want to support evicting the row cache when a region is closed, similar to how block cache eviction works, we need to include the region ID as part of the UID. Otherwise, it is impossible to determine which row cache entries are associated with the region being closed.

That's a good point. Let's keep it as it is now (region name + row key).

…Region

wchevreuil

LGTM, +1, just had some final nit comments.

wchevreuil · 2026-02-12T11:28:09Z

+ this.isRowCacheEnabled = determineRowCacheEnabled();
+ }
+
+ boolean determineRowCacheEnabled() {


nit: call it "checkRowCacheConfig()" and make it private.

wchevreuil · 2026-02-12T11:35:51Z

+ return false;
+ }
+
+ boolean tryGetFromCache(HRegion region, RowCacheKey key, Get get, List<Cell> results) {


nit: no need for the region param anymore.

wchevreuil · 2026-02-12T11:36:26Z

+ return true;
+ }
+
+ void populateCache(HRegion region, List<Cell> results, RowCacheKey key) {


nit: no need for the region param anymore.

EungsopYoo mentioned this pull request Oct 19, 2025
HBASE-29585 Add row-level cache for the get operation #7291
Open