-
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 3 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,35 @@ public long getFilterEntries() { | |
| // Estimate the number of entries as half the original file; this may be wildly inaccurate. | ||
| return super.getFilterEntries() / 2; | ||
| } | ||
|
|
||
| @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 |
|---|---|---|
|
|
@@ -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.