HBASE-28465 Implementation of framework for time-based priority bucket-cache #5793
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Apr 4, 2024
| long diff = currentTimestamp - maxTimestamp.getAsLong(); | ||
| return diff <= hotDataAge; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Shouldn't we return "true" by default? I.e., if not using DataTieringType.TIME_RANGE, this should consider all data as hot and let the LFU logic decide on the eviction right?
There was a problem hiding this comment.
The logic reaches that point only when the DataTieringType is set to NONE, indicating that data tiering is disabled. If data tiering is disabled, we should not consider that data as special by marking it as hot, correct? Instead, it should be considered as cold.
For example, if I set it to true, it means we are indicating that data is hot even when data tiering is disabled.
There was a problem hiding this comment.
For example, if I set it to true, it means we are indicating that data is hot even when data tiering is disabled.
Ok, I think there's a misunderstanding of concepts. Today, without this DataTiering thing, the behaviour is to cache everything and evict based on LFU (in other words, everything is hot). If I don't turn on DataTieringType.TIME_RANGE, I want things to keep working as today (cache everything and evict based on LFU). Maybe we can change the default name from NONE to ALL.
There was a problem hiding this comment.
Even with this implementation, it will function the same. For example, in eviction, we'll prioritize evicting the cold data files first, which are all the files that satisfy the condition (isDataTieringEnabled(file) && !isHotData(file)). Once I identify these files, I will proceed to evict them and then hand over control to the rest of the logic.
There was a problem hiding this comment.
Okay, I get your point. then we can eliminate isDataTieringEnabled methods, right? since we are considering everything as hot by default.
|
|
||
| HStoreFile hStoreFile = getHStoreFile(hFilePath); | ||
| if (hStoreFile == null) { | ||
| throw new DataTieringException( |
There was a problem hiding this comment.
Rather than throwing exception, maybe just return false? I wonder if it would be possible that a compaction completed and moved the given file away just after we entered this method.
There was a problem hiding this comment.
Wouldn't it be better to throw the exception and let the caller decide what action to take based on the context?
There was a problem hiding this comment.
I'm thinking in terms of responsibilities and cohesion. Who should know how to classify a block as hot or cold for a file that is not in the list of regions stores? Shouldn't this be done here?
There was a problem hiding this comment.
Got it. should we change in other places as well?
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DataTieringManager.java
Show resolved
Hide resolved
|
|
||
| try { | ||
| Set<String> coldFilePaths = dataTieringManager.getColdDataFiles(allCachedBlocks); | ||
| assertEquals(1, coldFilePaths.size()); |
There was a problem hiding this comment.
We should explain better why we only get 1 here, even though we say there are two non-hot files in the javadoc header.
Also, shouldn't we assert on the exact cold file, just to make sure our logic does mark the file we know is cold?
There was a problem hiding this comment.
Yeah, good idea. Maybe I write another test to stress on cold data
Sure, We'll add that. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Apr 5, 2024
| if (exception != null) { | ||
| fail("Expected DataTieringException to be thrown"); | ||
| } | ||
| assertEquals(value, expectedResult); |
There was a problem hiding this comment.
nit: assertEquals(expectedResult, value);
wchevreuil
reviewed
Apr 5, 2024
| if (exception != null) { | ||
| fail("Expected DataTieringException to be thrown"); | ||
| } | ||
| assertEquals(value, expectedResult); |
There was a problem hiding this comment.
nit: assertEquals(expectedResult, value);
|
💔 -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. |
|
LTGM, can we just address latest spotless failure? |
That's because of the Javadoc in the |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Apr 8, 2024
jhungund
reviewed
Apr 8, 2024
| if (dataTieringType.equals(DataTieringType.TIME_RANGE)) { | ||
| long hotDataAge = getDataTieringHotDataAge(configuration); | ||
|
|
||
| HStoreFile hStoreFile = getHStoreFile(hFilePath); |
There was a problem hiding this comment.
getConfiguratin(hFilePath) already traverses through the regions to get to the HStore. We will do it again within getHStoreFile. We can slightly improve by avoiding duplicate traversal to get HStore.
One Option I can think is to getStore first.
HStore store = getStore(hFilePath); <= We traverse only once.
Then,
Configuration configuration = getConfiguration(store);
HStoreFile file = getHStoreFile(store);
With this, we only have regions traversal only once.
There was a problem hiding this comment.
No, it doesn't traverse through the regions. these are just map lookups.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@wchevreuil, it seems like most of the tests have passed, and any failures were due to flaky tests. |
wchevreuil
pushed a commit
that referenced
this pull request
Apr 12, 2024
…t-cache (#5793) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Apr 22, 2024
…t-cache (#5793) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
Apr 25, 2024
…t-cache (#5793) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
May 20, 2024
…t-cache (#5793) Signed-off-by: Wellington Chevreuil <[email protected]>
vinayakphegde
added a commit
to vinayakphegde/hbase
that referenced
this pull request
May 21, 2024
…t-cache (apache#5793) Signed-off-by: Wellington Chevreuil <[email protected]>
wchevreuil
pushed a commit
that referenced
this pull request
May 21, 2024
…t-cache (#5793) Signed-off-by: Wellington Chevreuil <[email protected]>
jhungund
pushed a commit
to jhungund/hbase
that referenced
this pull request
May 29, 2024
…t-cache (apache#5793) Signed-off-by: Wellington Chevreuil <[email protected]> Change-Id: I22072f951ddfa8768e820952f63bbc2aa3cd5217
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.
No description provided.