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
| @@ -934,6 +936,27 @@ void freeSpace(final String why) { | |||
| } | |||
| 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?
| @@ -935,6 +937,7 @@ void freeSpace(final String why) { | |||
| } | |||
| 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.
| @@ -980,9 +991,18 @@ void freeSpace(final String why) { | |||
| BucketEntryGroup bucketMemory = | |||
| new BucketEntryGroup(bytesToFreeWithExtra, blockSize, getPartitionSize(memoryFactor)); | |||
|
|
|||
| long bytesFreed = 0; | |||
|
|
|||
| // Scan entire map putting bucket entry into appropriate bucket entry | ||
| // group | ||
| 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. |
|
💔 -1 overall
This message was automatically generated. |
Adjusted the log messages and add another unit test. Also applied spotless fixes. Change-Id: I0c6004b9dfe6ed7b169b8f1b6b8e7f1f38a9e591
jhungund
force-pushed
the
HBASE-28468_Alternative
branch
from
107d9f2 to
e5aaa04
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. |
wchevreuil
reviewed
Apr 23, 2024
Comment on lines
296
to
307
| // Add an additional block into cache with hot data which should trigger the eviction | ||
| BlockCacheKey newKey = new BlockCacheKey(hStoreFiles.get(2).getPath(), 0, true, BlockType.DATA); | ||
| CacheTestUtils.HFileBlockPair[] newBlock = CacheTestUtils.generateHFileBlocks(8192, 1); | ||
|
|
||
| bucketCache.cacheBlock(newKey, newBlock[0].getBlock()); | ||
| Waiter.waitFor(defaultConf, 1000000, 100, | ||
| () -> (bucketCache.getBackingMap().containsKey(newKey))); | ||
|
|
||
| // Verify that the bucket cache now contains 2 hot blocks blocks only. | ||
| // Both cold blocks of 8KB will be evicted to make room for 1 block of 8KB + an additional | ||
| // space. | ||
| validateBlocks(bucketCache.getBackingMap().keySet(), 2, 2, 0); |
There was a problem hiding this comment.
Why an eviction is expected here? Cache capacity is 64KB, the cache already had three blocks of 8KB and we are now adding a fourth 8KB size block, so total occupancy would be at 32KB, which should be ok, no?
There was a problem hiding this comment.
During my debugging I found that eviction does get triggered. However, I did reduce the size to 40K and the eviction still triggers.
There was a problem hiding this comment.
Yeah, we had reviewed this internally, not at all an issue. It's due to the internals of BucketAllocator when calculating buckets limits.
Comment on lines
289
to
291
| Waiter.waitFor(defaultConf, 1000000, 100, | ||
| () -> (bucketCache.getBackingMap().containsKey(key))); | ||
| } |
There was a problem hiding this comment.
Should we fail faster? Why this timeout needs to be so long?
There was a problem hiding this comment.
Yes, you are right. Let me bring it down to 10 secs timeout. I had set it to a high value while debugging it.
Comment on lines
347
to
354
| // Add an additional block into cache with hot data which should trigger the eviction | ||
| BlockCacheKey newKey = new BlockCacheKey(hStoreFiles.get(2).getPath(), 0, true, BlockType.DATA); | ||
| CacheTestUtils.HFileBlockPair[] newBlock = CacheTestUtils.generateHFileBlocks(8192, 1); | ||
|
|
||
| bucketCache.cacheBlock(newKey, newBlock[0].getBlock()); | ||
| Waiter.waitFor(defaultConf, 1000000, 100, | ||
| () -> (bucketCache.getBackingMap().containsKey(newKey))); | ||
|
|
There was a problem hiding this comment.
Similar to previous test. Why do we expect evictions when cache is less than 50% usage?
There was a problem hiding this comment.
I do see the eviction getting triggered.
Comment on lines
+370
to
+371
| BucketCache bucketCache = new BucketCache("file:" + testDir + "/bucket.cache", capacitySize, | ||
| 8192, bucketSizes, writeThreads, writerQLen, testDir + "/bucket.persistence", |
There was a problem hiding this comment.
The comment says it's creating a lower capacity cache, yet it's using same 64KB as in previous tests.
Comment on lines
395
to
401
| // Add an additional block which should evict the only cold block with an additional hot block. | ||
| BlockCacheKey newKey = new BlockCacheKey(hStoreFiles.get(2).getPath(), 0, true, BlockType.DATA); | ||
| CacheTestUtils.HFileBlockPair[] newBlock = CacheTestUtils.generateHFileBlocks(8192, 1); | ||
|
|
||
| bucketCache.cacheBlock(newKey, newBlock[0].getBlock()); | ||
| Waiter.waitFor(defaultConf, 1000000, 100, | ||
| () -> (bucketCache.getBackingMap().containsKey(newKey))); |
There was a problem hiding this comment.
Similar to previous tests. We expect evictions at less than 50% usage?
There was a problem hiding this comment.
I did see the evictions being triggered. I have reduced the bucket cache size to 40K bytes.
There was a problem hiding this comment.
Hi @wchevreuil, I varied the bucket cache sizes and checked the bucket cache numbers (capacity, free space, used space) at various stages in the unit test. Below are my observations.
For a bucket cache with 40K byes capacity:
At the start: Bucket Max Size: 40960, Current size: 0, Data size: 0, free size: 36864, Block Count: 0
After 1 block: Bucket Max Size: 40960, Current size: 9216, Data size: 8213, free size: 27648, Block Count:1
After 2 block: Bucket Max Size: 40960, Current size: 18432, Data size: 16426, free size: 18432, Block Count:2
After 3 block: Bucket Max Size: 40960, Current size: 27648, Data size: 24639, free size: 9216, Block Count:3
Eviction gets triggered for 4th block addition.
For a bucket cache with 64K capacity:
At the start: Bucket Max Size: 65536, Current size: 0, Data size: 0, free size: 36864, Block Count: 0
After 1 block: Bucket Max Size: 65536, Current size: 9216, Data size: 8213, free size: 27648, Block Count:1
After 2 block: Bucket Max Size: 65536, Current size: 18432, Data size: 16426, free size: 18432, Block Count:2
After 3 block: Bucket Max Size: 65536, Current size: 27648, Data size: 24639, free size: 9216, Block Count: 3
Eviction gets triggered for 4th block addition.
For a bucket cache with 72K capacity:
At the start: Bucket Max Size: 73728, Current size: 0, Data size: 0, free size: 73728, Block Count: 0
After 1 block: Bucket Max Size: 73728, Current size: 9216, Data size: 8213, free size: 64512, Block Count: 1
After 2 block: Bucket Max Size: 73728, Current size: 18432, Data size: 16426,free size: 55296, Block Count: 2
After 3 block: Bucket Max Size: 73728, Current size: 27648, Data size: 24639, free size: 46080,Block Count:3
Eviction does not get triggered for 4th Block addition.
Hence, as you can see that inspite of varying the bucket cache sizes between 40K and 64K, the free space available is only 36K. Hence, the eviction is triggered.
|
💔 -1 overall
This message was automatically generated. |
Also adjust the timeouts. Change-Id: I8e3e2132977514b49a9d3ad491c66680a71e5c31
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 <wchevreuil@apache.org>
wchevreuil
pushed a commit
that referenced
this pull request
May 20, 2024
…5829) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
May 21, 2024
…pache#5829) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil
pushed a commit
that referenced
this pull request
May 21, 2024
…5829) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
jhungund
added a commit
to janardhanrh/hbase
that referenced
this pull request
May 29, 2024
…pache#5829) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.