HBASE-29668 Add row cache framework#7398
Open
EungsopYoo wants to merge 1 commit intoapache:HBASE-29585from
Open
HBASE-29668 Add row cache framework#7398EungsopYoo wants to merge 1 commit intoapache:HBASE-29585from
EungsopYoo wants to merge 1 commit intoapache:HBASE-29585from
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
EungsopYoo
force-pushed
the
HBASE-29668
branch
from
f95570c to
cc96b4c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
|
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
force-pushed
the
HBASE-29668
branch
from
6136446 to
0df6d15
Compare
Contributor
Author
|
@wchevreuil |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
EungsopYoo
force-pushed
the
HBASE-29668
branch
from
c19a96d to
25cea27
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
EungsopYoo
force-pushed
the
HBASE-29668
branch
from
ea0e73a to
0df6d15
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@wchevreuil |
wchevreuil
reviewed
Jan 14, 2026
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
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).
Comment on lines
+45
to
+82
| @org.apache.yetus.audience.InterfaceAudience.Private | ||
| public class RowCacheService { | ||
| private final boolean enabledByConf; | ||
| private final RowCache rowCache; | ||
|
|
||
| RowCacheService(Configuration conf) { | ||
| enabledByConf = | ||
| conf.getFloat(HConstants.ROW_CACHE_SIZE_KEY, HConstants.ROW_CACHE_SIZE_DEFAULT) > 0; | ||
| rowCache = new RowCacheImpl(MemorySizeUtil.getRowCacheSize(conf)); | ||
| } | ||
|
|
||
| OperationStatus[] batchMutate(HRegion region, Mutation[] mArray, boolean atomic, long nonceGroup, | ||
| long nonce) throws IOException { | ||
| return region.batchMutate(mArray, atomic, nonceGroup, nonce); | ||
| } | ||
|
|
||
| BulkLoadHFileResponse bulkLoadHFile(RSRpcServices rsRpcServices, BulkLoadHFileRequest request) | ||
| throws ServiceException { | ||
| return rsRpcServices.bulkLoadHFileInternal(request); | ||
| } | ||
|
|
||
| CheckAndMutateResult checkAndMutate(HRegion region, CheckAndMutate checkAndMutate, | ||
| long nonceGroup, long nonce) throws IOException { | ||
| return region.checkAndMutate(checkAndMutate, nonceGroup, nonce); | ||
| } | ||
|
|
||
| RegionScannerImpl getScanner(HRegion region, Scan scan, List<Cell> results) throws IOException { | ||
| RegionScannerImpl scanner = region.getScanner(scan); | ||
| scanner.next(results); | ||
| return scanner; | ||
| } | ||
|
|
||
| Result mutate(RSRpcServices rsRpcServices, HRegion region, ClientProtos.MutationProto mutation, | ||
| OperationQuota quota, CellScanner cellScanner, long nonceGroup, | ||
| ActivePolicyEnforcement spaceQuotaEnforcement, RpcCallContext context) throws IOException { | ||
| return rsRpcServices.mutateInternal(mutation, region, quota, cellScanner, nonceGroup, | ||
| spaceQuotaEnforcement, context); | ||
| } |
Contributor
There was a problem hiding this comment.
Why do we need this class at all? It's adding a cyclic dependency within RSRpcServices. Can't we simply reference the RowCache impl from the HRegion instance?
| package org.apache.hadoop.hbase.regionserver; | ||
|
|
||
| @org.apache.yetus.audience.InterfaceAudience.Private | ||
| public class RowCacheImpl implements RowCache { |
Contributor
There was a problem hiding this comment.
nit: since we are not really doing anything here, can we remove this impl from this PR and focus only on the framework itself?
Contributor
Author
|
@wchevreuil |
Contributor
No problem. Enjoy your time off! |
Contributor
Author
|
I have a lot of backlog, so I think I’ll be able to resume in about one to two weeks. |
EungsopYoo
force-pushed
the
HBASE-29668
branch
from
0df6d15 to
193d215
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
EungsopYoo
force-pushed
the
HBASE-29668
branch
from
193d215 to
927b2e6
Compare
Contributor
Author
|
@wchevreuil Should I open a separate PR for the update, or could you take care of it directly? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.