Uh oh!
There was an error while loading. Please reload this page.
HBASE-28596 Optimise BucketCache usage upon regions splits/merges. - #5906
Conversation
Apache-HBase
commented
May 15, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 15, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 15, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 15, 2024
💔 -1 overall
This message was automatically generated. |
Change-Id: I510be0cb8041fead191f965178fd7336b6274352
Apache-HBase
commented
May 16, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Change-Id: Ib4454ed5de44ce35271e53a88caf7fce3b2d05ad
Apache-HBase
commented
May 17, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
In general, lack of comments and javadoc makes it a bit hard to review the changes...
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Change-Id: I970b6179aa4ccfe89dcd8eb7bb8eedbfc77037d5
Apache-HBase
commented
May 20, 2024
🎊 +1 overall
This message was automatically generated. |
Change-Id: I6dd46a801a44d9da8a824d10d57b88fb3c8cf0da
Change-Id: Ie893a0ef25bedc252a475b0f80f13beb28861e62
Change-Id: I8c88c9dd0eeefbb276c6d5f29b7c485df5482f3c
wchevreuil
commented
May 20, 2024
Thanks for the initial review, I have added extra comments and javadocs to help with reviewing. |
Apache-HBase
commented
May 20, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 20, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2024
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Jun 10, 2024
Just checked on these latest failures, I believe those are unrelated flakeys. |
| @Override | ||
| public void close(boolean evictOnClose) throws IOException { | ||
| if (closed.compareAndSet(false, true)) { | ||
| if (evictOnClose && StoreFileInfo.isReference(this.reader.getPath())) { |
There was a problem hiding this comment.
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.
Yeah, I think there's no need for this extra check, as we'll always have a reference with HalfStoreFileReader. Let me remove this.
| } | ||
| /** | ||
| * Similarly to HFileBlock.Writer.getBlockForCaching(), creates a HFileBlock instance without |
There was a problem hiding this comment.
Could we share some common code between these two methods? Otherwise how can we align these two methods?
There was a problem hiding this comment.
Yes, let me work on this.
There was a problem hiding this comment.
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.
| // Prefetch file blocks upon open if requested | ||
| if (cacheConf.shouldPrefetchOnOpen() && cacheIfCompactionsOff() && shouldCache.booleanValue()) { | ||
| if (cacheConf.shouldPrefetchOnOpen() && shouldCache.booleanValue()) { |
There was a problem hiding this comment.
Why removing cacheIfCompactionsOff?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Better add some comments to explain the history here?
| public void close(boolean evictOnClose) throws IOException { | ||
| if (closed.compareAndSet(false, true)) { | ||
| if (evictOnClose && StoreFileInfo.isReference(this.reader.getPath())) { | ||
| final HFileReaderImpl.HFileScannerImpl s = |
There was a problem hiding this comment.
Usually it is not a good idea to the implementation class directly at upper layer. Why we need to use HFileScannerImpl here?
There was a problem hiding this comment.
We need to seek to the split cell, in order to calculate offsets for the cache keys of referred file to evict.
There was a problem hiding this comment.
But there is a seekTo method in HFileScanner interface? I just mean why we need the cast here?
There was a problem hiding this comment.
I need the HFileScannerImpl.getCurBlock for figuring out the offset.
Uh oh!
There was an error while loading. Please reload this page.
Change-Id: If712a4310a8f651b2395dd233fec7f8430362da5
Apache-HBase
commented
Jun 12, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 12, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 12, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 12, 2024
💔 -1 overall
This message was automatically generated. |
Change-Id: I480a6a4f062d1d57d8a432d93d9f9c4e62c9c826
Apache-HBase
commented
Jun 13, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 13, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 13, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 13, 2024
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Jun 14, 2024
Any further concerns, @Apache9 ? |
…5906) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
…5906) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
…pache#5906) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org> Change-Id: I6d6fc84c0e526cc1ed1c14072791b0bd5b9b92fa
…ons splits/merges. (apache#5906) (apache#98) Reviewed-by: Duo Zhang <zhangduo@apache.org> Change-Id: Ic673b3264b6bbc7bdb681d1297bdf827b6906e03 Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Co-authored-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> (cherry picked from commit d34e5c3)
…pache#5906) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org> Change-Id: I6d6fc84c0e526cc1ed1c14072791b0bd5b9b92fa
…ons splits/merges. (apache#5906) (apache#98) Reviewed-by: Duo Zhang <zhangduo@apache.org> Change-Id: Ic673b3264b6bbc7bdb681d1297bdf827b6906e03 Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Co-authored-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> (cherry picked from commit d34e5c3)
No description provided.