Uh oh!
There was an error while loading. Please reload this page.
HBASE-23066 Allow cache on write during compactions when prefetching … - #707
HBASE-23066 Allow cache on write during compactions when prefetching …#707jacob-leblanc wants to merge 1 commit into
Conversation
Apache-HBase
commented
Oct 10, 2019
🎊 +1 overall
This message was automatically generated. |
ramkrish86
commented
Oct 14, 2019
LGTM. Let's wait for some time for others to review. |
| * Configuration key to cache blocks when a compacted file is written, predicated on prefetching | ||
| * being enabled for the column family. | ||
| */ | ||
| public static final String PREFETCH_COMPACTED_BLOCKS_ON_WRITE_KEY = |
There was a problem hiding this comment.
A bit confusing.. Are we doing the prefetch of the new compacted file once it is written?
Dont think so.. When we write the file, that time itself the caching happens. So it is cache on write. Why its called prefetch then? There is no extra fetch op happening right?
| * @return true if blocks should be cached while writing during compaction, false if not | ||
| */ | ||
| public boolean shouldCacheCompactedBlocksOnWrite() { | ||
| return this.prefetchCompactedDataOnWrite && this.prefetchOnOpen; |
There was a problem hiding this comment.
Oh.. So the cache on write (at compaction) happens iff prefetch config is ON ! Anyways in ur case the prefetch which is another config, is ON right? I think this is the reason why the new config you have named that way. But some how I feel that config name is bit misleading.
There was a problem hiding this comment.
Actually the cache size should be much bigger than the hot data set size if u want to do cache on compact. Because the compacted away data might be already in cache (Those are flused files or a result of another compaction). Those are recently been accessed also (by the compaction thread). This feature should be very carefully used.
There was a problem hiding this comment.
Thanks for looking at this. My understanding is that in cases where prefetch is enabled, the new file is going to be read into the cache after compaction completes anyway. So the cache size requirements are the same when this new setting is enabled. This is why I wanted to limit the scope of the cache on write to only apply where prefetching is enabled: it simply is a way to do the cache loading more efficiently while we are writing the data out rather than having to read it back after compaction is done which I've found is very expensive when data is in S3.
As far as the name goes, I struggled to come up with something intuitive - how do I explain in the name alone that this only applies when prefetching is on? I tried to convey "when prefetching, do the prefetch of compacted data on write." I'm not in love with the name and I'm open to suggestions. I didn't want to give the false impression that all compacted data is going to be cached on write. Maybe "cacheCompactedDataOnWriteIfPrefetching"? Is that too wordy?
busbey
commented
Dec 11, 2019
is this made obsolete by #919? |
virajjasani
commented
Feb 23, 2020
I believe so. Closing this PR @jacob-leblanc@ramkrish86 |
…is enabled