EungsopYoo commented on PR #7291:
URL: https://github.com/apache/hbase/pull/7291#issuecomment-3283057517
> This is a great idea, thanks for sharing it. I do have some comments,
though:
Thank you for starting the PR review.
> 1. Can the RowCacheService be an implementation of BlockCache? Maybe a
wrapper to the LRUBlockCache. I'm a bit worried about introducing a whole new
layer for intercepting all read/write operations at the RPC service with cache
specific logic, however this class is not the cache implementation itself.
Seems a bit confusing to have a complete separate entry point to the cache.
BlockCache operates at the HFile access layer, whereas the row cache needs
to function at a higher layer that covers both MemStore and HFile. That’s why I
implemented RowCacheService in the RPC service layer.
That said, the row cache does not actually cache HFileBlocks, yet it
currently relies on the BlockCache interface. I realize this might not be
appropriate. I reused the BlockCache interface to reduce the overhead of
creating a separate cache implementation solely for the row cache, but in
hindsight, this might not have been the best approach. It may be better to
build a dedicated cache implementation specifically for the row cache.
What do you think?
> 2. Are we accepting to have same row data in multiple cache? In the
current code, I haven't see any checks to avoid that. Maybe if we implement
RowCacheService as a block cache implementation, so that the cache operations
happen from the inner layers of the read/write operations, it would be easier
to avoid duplication.
What exactly does “multiple cache” refer to? Does it mean the L1 and L2
caches in the CombinedBlockCache? If so, I haven’t really considered that
aspect yet, but I’ll start looking into it.
> 3. Why not simply evict the row that got mutated? I guess we cannot simply
override it in the cache because mutation can happen on individual cells.
I didn’t fully understand the intention behind your question. Could you
please explain it in more detail?
> 4. Are we accepting to have data duplicated over separate caches? I don't
see any logic to avoid caching a whole block containing a region for a Get in
the L2 cache, still we'll be cache the row in the row cache. Similarly, we
might re-cache a row that's in the memstore in the row cache.
This is in the same L1/L2 context as your comment 2, correct? If so, I
haven’t considered that aspect yet, but I’ll start thinking about how to handle
it.
Since the row cache is only enabled when there are at least two HFiles, rows
that exist only in the MemStore are not cached. However, when there are two or
more HFiles, rows in MemStore are also added again to the row cache. This is an
intentional design choice, aimed at bypassing the process of generating results
via SegmentScanner and StoreFileScanner, and instead serving Get requests
directly from the cache.
> 5. One problem of adding such small units (a single row) in the cache is
that we need to keep a map index for each entry. So, the smaller the row in
size, more rows would fit in the cache, but more key objects would be retained
in the map. In your tests, assuming the default block cache size of 40% of the
heap, it would give a 12.8GB of block cache. Have you managed to measure the
block cache usage by the row cache, in terms of number of rows in the cache,
byte size of the L1 cache and the total heap usage? Maybe wort collecting a
heapdump to analyse the map index size in the heap.
I slightly modified the LruBlockCache code to record the row cache size and
entry count. The row cache occupies 268.67MB with 338,602 entries. The average
size of a single row cache entry is 830 bytes. Within the overall BlockCache,
the row cache accounts for 45% by entry count and 2% by size.
```
2025-09-12T09:08:44,112 INFO [LruBlockCacheStatsExecutor {}]
hfile.LruBlockCache: totalSize=12.80 GB, usedSize=12.48 GB, freeSize=329.41 MB,
max=12.80 GB, blockCount=752084, accesses=35942999, hits=27403857,
hitRatio=76.24%, , cachingAccesses=35942954, cachingHits=27403860,
cachingHitsRatio=76.24%, evictions=170, evicted=5806436,
evictedPerRun=34155.50588235294, rowBlockCount=338602, rowBlockSize=268.67 MB
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
