HBASE-28468: Integrate the data-tiering logic into cache evictions. #5829
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhungund
commented
Apr 15, 2024
| try { | ||
| freeInProgress = true; | ||
|
|
||
| // Fetch the file names from the backing map. |
There was a problem hiding this comment.
The main logic is to rely on the file names. Fetch the file names from the block cache keys.
Pass the list to a new DataTieringManager API to detect which of these files are cold-files and return the list of cold files.
There was a problem hiding this comment.
We can use the BlocksByFile instead of BackingMap if it seems more efficient.
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
from
2fa96c5 to
5998601
Compare
|
💔 -1 overall
This message was automatically generated. |
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
from
5998601 to
2f59db2
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
2 times, most recently
from
47ec22d to
836a70f
Compare
jhungund
changed the title
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
from
836a70f to
a357c63
Compare
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
requested changes
Apr 18, 2024
There was a problem hiding this comment.
Overall, I like this approach better than the previous proposed in PR #5826. The only con is the overhead in figuring out the "cold" files within the eviction process. As discussed internally, we could add an extra global config for when data tiering manager should be considered.
Additional ask I have is to add a UT for this modified behaviour in freespace(). We should test that eviction correctly picks blocks for cold files but leave the hot ones.
| if (!maxTimestamp.isPresent()) { | ||
| // We could throw from here, But we are already in the critical code-path | ||
| // of freeing space. Hence, we can ignore that file for now | ||
| // Or do we want to include it? |
There was a problem hiding this comment.
That's fine for me. Can we add a debug/trace log?
| } | ||
| try { | ||
| freeInProgress = true; | ||
|
|
There was a problem hiding this comment.
nit: please avoid unnecessary new lines.
Comment on lines
984
to
985
| // List<BlockCacheKey> coldBlocks = new ArrayList<>(); | ||
|
|
There was a problem hiding this comment.
nit: please remove commented code.
| new BucketEntryGroup(bytesToFreeWithExtra, blockSize, getPartitionSize(memoryFactor)); | ||
|
|
||
| long bytesFreed = 0; | ||
|
|
| for (Map.Entry<BlockCacheKey, BucketEntry> bucketEntryWithKey : backingMap.entrySet()) { | ||
|
|
||
| if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName())) { | ||
| //coldBlocks.add(bucketEntryWithKey.getKey()); |
There was a problem hiding this comment.
nit: remove commented code.
| if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName())) { | ||
| //coldBlocks.add(bucketEntryWithKey.getKey()); | ||
| bytesFreed += bucketEntryWithKey.getValue().getLength(); | ||
| evictBlock(bucketEntryWithKey.getKey()); |
There was a problem hiding this comment.
We are on an eviction process, this call would miss the proper stats counting. Also, we should care about ref counts, so better use evictBucketEntryIfNoRpcReferenced for this eviction.
Also here we are evicting all blocks for files considered cold. Shouldn't we do it only until we reach the bytesToFreeWithoutExtramark?
There was a problem hiding this comment.
You mean we need to evict these cold file blocks even before the calculations for bytesToFreeWithoutExtra? That will require one additional traversal of backingMap. I will incorporate this change and we can take a look if that looks ok.
There was a problem hiding this comment.
No, that not what I meant. You already have bytesToFreeWithExtra calculated in line #983 and you are computing bytesFreed. Once bytesFreed reached bytesToFreeWithExtra value, you still want to evict the cold files or should we just interrupt the loop?
|
|
||
| if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName())) { | ||
| //coldBlocks.add(bucketEntryWithKey.getKey()); | ||
| bytesFreed += bucketEntryWithKey.getValue().getLength(); |
There was a problem hiding this comment.
We should only account for this once the eviction was indeed successfull.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
3 times, most recently
from
0d9e81d to
3a8c0dc
Compare
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Apr 19, 2024
| if (coldFiles != null | ||
| && coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName())) { | ||
| int freedBlockSize = bucketEntryWithKey.getValue().getLength(); | ||
| evictBlockIfNoRpcReferenced(bucketEntryWithKey.getKey()); |
There was a problem hiding this comment.
evictBlockIfNoRpcReferenced returns a boolean indicating whether the eviction was successful or not. We should check that boolean in orther to decide if we should increment bytesFreed.
| if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName())) { | ||
| //coldBlocks.add(bucketEntryWithKey.getKey()); | ||
| bytesFreed += bucketEntryWithKey.getValue().getLength(); | ||
| evictBlock(bucketEntryWithKey.getKey()); |
There was a problem hiding this comment.
No, that not what I meant. You already have bytesToFreeWithExtra calculated in line #983 and you are computing bytesFreed. Once bytesFreed reached bytesToFreeWithExtra value, you still want to evict the cold files or should we just interrupt the loop?
jhungund
commented
Apr 19, 2024
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
Show resolved
Hide resolved
wchevreuil
reviewed
Apr 19, 2024
Comment on lines
267
to
294
| // Verify that the bucket cache contains 4 blocks. | ||
| assertEquals(3, bucketCache.getBackingMap().keySet().size()); |
There was a problem hiding this comment.
nit: comment mismatching the code.
Comment on lines
262
to
263
| while (!(bucketCache.getBackingMap().containsKey(key))) { | ||
| Thread.sleep(100); |
There was a problem hiding this comment.
nit: use Waiter.waitFor
Comment on lines
275
to
277
| while (!(bucketCache.getBackingMap().containsKey(newKey))) { | ||
| Thread.sleep(100); | ||
| } |
There was a problem hiding this comment.
nit: use Waiter.waitFor
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
from
79fc1ec to
0444e84
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
retest |
Adjust the unit test to incorporate the delay in the prefetch due to metadata access. Change-Id: I2fc25d564be0aa454921cafa5ce40d9df8135211
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Apr 25, 2024
wchevreuil
pushed a commit
that referenced
this pull request
Apr 25, 2024
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
May 20, 2024
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
May 21, 2024
…pache#5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
May 21, 2024
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
jhungund
added a commit
to janardhanrh/hbase
that referenced
this pull request
May 29, 2024
…pache#5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Jun 24, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Jul 10, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Jul 11, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Aug 5, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Sep 1, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Sep 1, 2025
…pache#5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Sep 1, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Sep 3, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Sep 3, 2025
…pache#5829) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Sep 3, 2025
…5829) Signed-off-by: Wellington Chevreuil <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a part of cache blocks eviction when the cache is full,
the data-tiering logic is integrated into the freeSpace code path
to identify the cold data files and evict the blocks associated with
those files.
The list of files is traversed to identify the cold files based
on the hot-data-age configuration and also the max timestamp associated
with the files.
The blocks associated with those cold files are evicted first and then
the existing logic of LFU blocks is executed to further evict the blocks.