-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28596 Optimise BucketCache usage upon regions splits/merges. #5906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
3d71529
7bf567f
1f9d91f
61a12fc
8bb7507
ca03c50
96ebf97
3c87622
4587713
64b0855
243df04
7dba9bf
a23ca87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.function.IntConsumer; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.hbase.Cell; | ||
|
|
@@ -29,6 +30,7 @@ | |
| import org.apache.hadoop.hbase.client.Scan; | ||
| import org.apache.hadoop.hbase.io.hfile.CacheConfig; | ||
| import org.apache.hadoop.hbase.io.hfile.HFileInfo; | ||
| import org.apache.hadoop.hbase.io.hfile.HFileReaderImpl; | ||
| import org.apache.hadoop.hbase.io.hfile.HFileScanner; | ||
| import org.apache.hadoop.hbase.io.hfile.ReaderContext; | ||
| import org.apache.hadoop.hbase.regionserver.StoreFileInfo; | ||
|
|
@@ -64,6 +66,8 @@ public class HalfStoreFileReader extends StoreFileReader { | |
|
|
||
| private boolean firstKeySeeked = false; | ||
|
|
||
| private AtomicBoolean closed = new AtomicBoolean(false); | ||
|
|
||
| /** | ||
| * Creates a half file reader for a hfile referred to by an hfilelink. | ||
| * @param context Reader context info | ||
|
|
@@ -349,4 +353,42 @@ public long getFilterEntries() { | |
| // Estimate the number of entries as half the original file; this may be wildly inaccurate. | ||
| return super.getFilterEntries() / 2; | ||
| } | ||
|
|
||
| /** | ||
| * Overrides close method to handle cache evictions for the referred file. If evictionOnClose is | ||
| * true, we will seek to the block containing the splitCell and evict all blocks from offset 0 up | ||
| * to that block offset if this is a bottom half reader, or the from the split block offset up to | ||
| * the end of the file if this is a top half reader. | ||
| * @param evictOnClose true if it should evict the file blocks from the cache. | ||
| */ | ||
| @Override | ||
| public void close(boolean evictOnClose) throws IOException { | ||
| if (closed.compareAndSet(false, true)) { | ||
| if (evictOnClose && StoreFileInfo.isReference(this.reader.getPath())) { | ||
| final HFileReaderImpl.HFileScannerImpl s = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually it is not a good idea to the implementation class directly at upper layer. Why we need to use HFileScannerImpl here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to seek to the split cell, in order to calculate offsets for the cache keys of referred file to evict.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But there is a seekTo method in HFileScanner interface? I just mean why we need the cast here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need the HFileScannerImpl.getCurBlock for figuring out the offset. |
||
| (HFileReaderImpl.HFileScannerImpl) super.getScanner(false, true, false); | ||
| final String reference = this.reader.getHFileInfo().getHFileContext().getHFileName(); | ||
| final String referred = StoreFileInfo.getReferredToRegionAndFile(reference).getSecond(); | ||
| s.seekTo(splitCell); | ||
| if (s.getCurBlock() != null) { | ||
| long offset = s.getCurBlock().getOffset(); | ||
| LOG.trace("Seeking to split cell in reader: {} for file: {} top: {}, split offset: {}", | ||
| this, reference, top, offset); | ||
| ((HFileReaderImpl) reader).getCacheConf().getBlockCache().ifPresent(cache -> { | ||
| int numEvictedReferred = top | ||
| ? cache.evictBlocksRangeByHfileName(referred, offset, Long.MAX_VALUE) | ||
| : cache.evictBlocksRangeByHfileName(referred, 0, offset); | ||
| int numEvictedReference = cache.evictBlocksByHfileName(reference); | ||
| LOG.trace( | ||
| "Closing reference: {}; referred file: {}; was top? {}; evicted for referred: {};" | ||
| + "evicted for reference: {}", | ||
| reference, referred, top, numEvictedReferred, numEvictedReference); | ||
| }); | ||
| } | ||
| reader.close(false); | ||
| } else { | ||
| reader.close(evictOnClose); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ | |
| */ | ||
| package org.apache.hadoop.hbase.io.hfile; | ||
|
|
||
| import static org.apache.hadoop.hbase.io.hfile.HFileBlock.FILL_HEADER; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.util.NavigableMap; | ||
|
|
@@ -25,7 +27,9 @@ | |
| import java.util.concurrent.ConcurrentSkipListSet; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.hbase.metrics.impl.FastLongHistogram; | ||
| import org.apache.hadoop.hbase.nio.ByteBuff; | ||
| import org.apache.hadoop.hbase.util.Bytes; | ||
| import org.apache.hadoop.hbase.util.ChecksumType; | ||
| import org.apache.hadoop.hbase.util.GsonUtil; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
| import org.slf4j.Logger; | ||
|
|
@@ -244,6 +248,45 @@ public static int getMaxCachedBlocksByFile(Configuration conf) { | |
| return conf == null ? DEFAULT_MAX : conf.getInt("hbase.ui.blockcache.by.file.max", DEFAULT_MAX); | ||
| } | ||
|
|
||
| /** | ||
| * Similarly to HFileBlock.Writer.getBlockForCaching(), creates a HFileBlock instance without | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we share some common code between these two methods? Otherwise how can we align these two methods?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let me work on this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the most I could reuse here is the new HFIleContext part. The block builder portion is hard to reuse because in HFileBlock.Writer the parameters are calculated on the go, so any sort of reuse would require an awful lot of parameters. |
||
| * checksum for caching. This is needed for when we cache blocks via readers (either prefetch or | ||
| * client read), otherwise we may fail equality comparison when checking against same block that | ||
| * may already have been cached at write time. | ||
| * @param cacheConf the related CacheConfig object. | ||
| * @param block the HFileBlock instance to be converted. | ||
| * @return the resulting HFileBlock instance without checksum. | ||
| */ | ||
| public static HFileBlock getBlockForCaching(CacheConfig cacheConf, HFileBlock block) { | ||
| HFileContext newContext = | ||
| new HFileContextBuilder().withBlockSize(block.getHFileContext().getBlocksize()) | ||
| .withBytesPerCheckSum(0).withChecksumType(ChecksumType.NULL) // no checksums in cached data | ||
| .withCompression(block.getHFileContext().getCompression()) | ||
| .withDataBlockEncoding(block.getHFileContext().getDataBlockEncoding()) | ||
| .withHBaseCheckSum(block.getHFileContext().isUseHBaseChecksum()) | ||
| .withCompressTags(block.getHFileContext().isCompressTags()) | ||
| .withIncludesMvcc(block.getHFileContext().isIncludesMvcc()) | ||
| .withIncludesTags(block.getHFileContext().isIncludesTags()) | ||
| .withColumnFamily(block.getHFileContext().getColumnFamily()) | ||
| .withTableName(block.getHFileContext().getTableName()).build(); | ||
| // Build the HFileBlock. | ||
| HFileBlockBuilder builder = new HFileBlockBuilder(); | ||
| ByteBuff buff = block.getBufferReadOnly(); | ||
| // Calculate how many bytes we need for checksum on the tail of the block. | ||
| int numBytes = cacheConf.shouldCacheCompressed(block.getBlockType().getCategory()) | ||
| ? 0 | ||
| : (int) ChecksumUtil.numBytes(block.getOnDiskDataSizeWithHeader(), | ||
| block.getHFileContext().getBytesPerChecksum()); | ||
| return builder.withBlockType(block.getBlockType()) | ||
| .withOnDiskSizeWithoutHeader(block.getOnDiskSizeWithoutHeader()) | ||
| .withUncompressedSizeWithoutHeader(block.getUncompressedSizeWithoutHeader()) | ||
| .withPrevBlockOffset(block.getPrevBlockOffset()).withByteBuff(buff) | ||
| .withFillHeader(FILL_HEADER).withOffset(block.getOffset()).withNextBlockOnDiskSize(-1) | ||
| .withOnDiskDataSizeWithHeader(block.getOnDiskDataSizeWithHeader() + numBytes) | ||
| .withHFileContext(newContext).withByteBuffAllocator(cacheConf.getByteBuffAllocator()) | ||
| .withShared(!buff.hasArray()).build(); | ||
| } | ||
|
|
||
| /** | ||
| * Use one of these to keep a running account of cached blocks by file. Throw it away when done. | ||
| * This is different than metrics in that it is stats on current state of a cache. See | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ public HFilePreadReader(ReaderContext context, HFileInfo fileInfo, CacheConfig c | |
| }); | ||
|
|
||
| // Prefetch file blocks upon open if requested | ||
| if (cacheConf.shouldPrefetchOnOpen() && cacheIfCompactionsOff() && shouldCache.booleanValue()) { | ||
| if (cacheConf.shouldPrefetchOnOpen() && shouldCache.booleanValue()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why removing cacheIfCompactionsOff?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason as explained here: https://github.com/apache/hbase/pull/5906/files#r1606971440. We want the prefetch to run even for refs or links, the cache implementation should decide what to do with refs/links.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better add some comments to explain the history here? |
||
| PrefetchExecutor.request(path, new Runnable() { | ||
| @Override | ||
| public void run() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to check isReference here? We will only use HalfStoreFileReader when the store file is a reference file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think there's no need for this extra check, as we'll always have a reference with HalfStoreFileReader. Let me remove this.