wchevreuil commented on PR #7291:
URL: https://github.com/apache/hbase/pull/7291#issuecomment-3285200841
> 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?
>
Yeah, I had the same thought while going through the comments. Having a
separate cache structure seems the best way to implement this.
> > 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.
>
Nevermind my previous comment. We should focus on the separate cache for
rows.
> > 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?
>
Rather than blocking writes to the row cache during updates/bulkload, can we
simply make the updates evict/override the row from the cache if it's already
there? For puts, we shouldn't need to worry about barries, if we make sure we
don't cache the row if it's in the memstore only, but we should to make sure to
remove it from the row cache because the cache would now be stale. For
bulkloads, I guess we only need to make sure to evict the rows for affected
regions after the bulkload has been committed.
> > 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.
>
Per other comments, agree it's fine to have the row in the row cache and
its' block also in the block cache. We need to decide if we want to add blocks
to the block cache when doing Get, or Get should cache only in the row cache?
Also, should we avoid caching if the row is the memstore? Could be challenging
in the current design of caching the whole row, because memstore migh have only
updates for few cells within a row.
> > 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
> ```
What if more rows get cached, over time, as more gets for different rows are
executed? It could lead to many rows in the cache, and many more objects in the
map to index it. In the recent past. we've seen some heap issues when having
very large file based bucket cache and small compressed blocks. I guess we
could face similar problems here too.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
