HBASE-28535: Add a region-server wide key to enable data-tiering. #5856
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
jhungund
force-pushed
the
HBASE-28535
branch
2 times, most recently
from
0ecde23 to
6029543
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Data-tiering feature should not be enabled by default, since this feature has specific use-cases. Hence, introduce a system-wide configuration to enable the feature. Avoid the code data-tiering code paths when this system-wide configuration is not enabled. Change-Id: I308119884c42173cfef3b23c360a842c7f516977
|
🎊 +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. |
| // Initialize the metadata only if the data-tiering is enabled. | ||
| // If not, the metadata will be initialized later. | ||
| fileInfo.initMetaAndIndex(this); | ||
| } |
There was a problem hiding this comment.
I don't think this part is needed. We decided not to use Data Tiering logic here and restrict it to Bucket Cache, which I will take care of in the refactoring JIRA. Additionally, if the concern is to avoid executing initMetaAndIndex twice, then I have modified initMetaAndIndex to only execute once.
There was a problem hiding this comment.
This check is to avoid any changes to the old functionality. We had observed that this line does introduce some performance regression leading to a unit test failure. We keep the old behaviour to initialise the metadata at the original location when data-tiering is not enabled.
There was a problem hiding this comment.
Let us please avoid spaghetti code and keep components cohesive. We shall not pollute the reader with data tiering logic. As @vinayakphegde mentioned, initiMetaAndIndex already knows it should perform init logic only once in the flow, so this call here won't add extra execution.
There was a problem hiding this comment.
ok, I will remove the check here. The intention was to keep the old behaviour when the feature is disabled.
| @@ -45,6 +45,8 @@ | |||
| @InterfaceAudience.Private | |||
| public class DataTieringManager { | |||
| private static final Logger LOG = LoggerFactory.getLogger(DataTieringManager.class); | |||
| public static final String DATA_TIERING_ENABLED_KEY = "hbase.hstore.datatiering.enable"; | |||
There was a problem hiding this comment.
Since it is a cluster-wide (or region server-level) configuration, I think the format should be something like hbase.regionserver.*, although I'm not entirely sure. Additionally, could we use a better name for the variable to indicate that this is a cluster-wide configuration? like, GLOBAL_DATA_TIERING_ENABLED_KEY?
| if (instance == null) { | ||
| instance = new DataTieringManager(onlineRegions); | ||
| LOG.info("DataTieringManager instantiated successfully."); | ||
| public static synchronized void instantiate(Configuration conf, |
There was a problem hiding this comment.
Why do we need this Configuration parameter? We only enter here when isDataTieringFeatureEnabled(conf) is true, so why do we need to check it again?
There was a problem hiding this comment.
This is an additional check to avoid an unintentional use if someone makes a call to this function.
There was a problem hiding this comment.
If you are enforcing the check here, than there's no point in exposing isDataTieringFeatureEnabled as a public method, as you are not giving any flexibility to clients on what to do upon the return of isDataTieringFeatureEnabled.
There was a problem hiding this comment.
I have made the function isDataTieringFeatureEnabled private function of the class. We do not want to give the undue flexiblity to the user to instantiate the DataTieringManager even when the feature key is disabled.
There was a problem hiding this comment.
Also, would it make sense to return True if instantiated successfully and false otherwise?
| DataTieringManager.instantiate(onlineRegions); | ||
| if (DataTieringManager.isDataTieringFeatureEnabled(conf)) { | ||
| DataTieringManager.instantiate(conf, onlineRegions); | ||
| } |
There was a problem hiding this comment.
Since we are not instantiating the DataTieringManager itself when global configuration is not enabled, do we need to restart every time we change that global parameter?
There was a problem hiding this comment.
Yes, we will require server restart since we are initialising the DataTieringManager during server restart.
wchevreuil
requested changes
Apr 29, 2024
There was a problem hiding this comment.
I don't understand why this change has required to disable PREFETCH_BLOCKS_ON_OPEN_KEY configuration on all UTs. Can you explain why? And since you are doing for all tests, why not leave it disabled?
Additionally, can we add test for the modified behaviour itself? I don't see any tests for the modified behaviour.
| // Initialize the metadata only if the data-tiering is enabled. | ||
| // If not, the metadata will be initialized later. | ||
| fileInfo.initMetaAndIndex(this); | ||
| } |
There was a problem hiding this comment.
Let us please avoid spaghetti code and keep components cohesive. We shall not pollute the reader with data tiering logic. As @vinayakphegde mentioned, initiMetaAndIndex already knows it should perform init logic only once in the flow, so this call here won't add extra execution.
Comment on lines
2202
to
2203
| } else { | ||
| LOG.debug("Data tiering is enabled for file: '{}' and it is hot data", fileName); |
There was a problem hiding this comment.
Nit: no need for this extra else block, just do the logging.
Comment on lines
2205
to
2207
| } else { | ||
| LOG.debug("Data tiering feature is not enabled. " | ||
| + " The file: '{}' will be loaded if not already loaded", fileName); |
There was a problem hiding this comment.
Nit: no need for this extra else block, just do the logging.
| } | ||
|
|
||
| // if we don't have the file in fullyCachedFiles, we should cache it | ||
| // if we don't have the file in fullyCachedFiles, we should cache it. |
There was a problem hiding this comment.
Nit: avoid useless line changes in the PR.
| if (instance == null) { | ||
| instance = new DataTieringManager(onlineRegions); | ||
| LOG.info("DataTieringManager instantiated successfully."); | ||
| public static synchronized void instantiate(Configuration conf, |
There was a problem hiding this comment.
If you are enforcing the check here, than there's no point in exposing isDataTieringFeatureEnabled as a public method, as you are not giving any flexibility to clients on what to do upon the return of isDataTieringFeatureEnabled.
Comment on lines
537
to
539
| if (DataTieringManager.isDataTieringFeatureEnabled(conf)) { | ||
| DataTieringManager.instantiate(conf, onlineRegions); | ||
| } |
There was a problem hiding this comment.
No need for this if check, since you do it internally on DataTieringManager.instantiate. If you want to keep the check inside DataTieringManager.instantiate, then no need for this check here.
| Waiter.waitFor(defaultConf, 10000, 100, () -> (bucketCache.getBackingMap().containsKey(key))); | ||
| } | ||
| // disable any prefetch in parallel to test execution | ||
| defaultConf.setBoolean(CacheConfig.PREFETCH_BLOCKS_ON_OPEN_KEY, false); |
There was a problem hiding this comment.
Is this necessary because of the modified functionality? Otherwise, can you explain why this is needed now?
There was a problem hiding this comment.
There have been intermittent failures in these tests where the block counts different than expected at different points. I did observe the prefetch kicking in during those failures. Hence, I tried to fix these failures. But I realise, it is better to keep these test fixes different from this original change. I will remove these changes in the new patch set.
|
💔 -1 overall
This message was automatically generated. |
jhungund
force-pushed
the
HBASE-28535
branch
2 times, most recently
from
ab8de8e to
825e026
Compare
|
🎊 +1 overall
This message was automatically generated. |
jhungund
commented
Apr 29, 2024
| Waiter.waitFor(defaultConf, 10000, 100, | ||
| () -> (bucketCache.getBackingMap().containsKey(newKey))); | ||
|
|
||
| // Verify that the bucket still contains the 2 cold blocks and one newly added hot block. |
There was a problem hiding this comment.
Currently, in this test, which blocks stay is not deterministic. Any block may get evicted leading to test failure.
@wchevreuil, @vinayakphegde, any idea to enforce the eviction of hot blocks and retain the cold blocks in cache to validate that data-tiering is not effective? If not, we may have to rely on the absence of DataTieringManager instance only.
|
💔 -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-28535
branch
4 times, most recently
from
f076005 to
363a071
Compare
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I95ec8691f5d511a8bd452c1492de7ff1222980b6
jhungund
commented
Apr 30, 2024
|
|
||
| int blocksIter = 0; | ||
| for (BlockCacheKey key : cacheKeys) { | ||
| LOG.info("Adding {}", key); |
There was a problem hiding this comment.
The test seems to work with the latest fixes. However, I am keeping these logs handy until the flakyness is fixed.
|
🎊 +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
approved these changes
May 2, 2024
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
May 21, 2024
…ache#5856) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
jhungund
added a commit
to jhungund/hbase
that referenced
this pull request
May 29, 2024
…ache#5856) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Change-Id: If9851a2a748bf24674ee1819487ce6ee8e75e7a0
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.
Data-tiering feature should not be enabled by default, since this feature has specific use-cases. Hence, introduce a system-wide configuration to enable the feature.
Avoid the code data-tiering code paths when this system-wide configuration is not enabled.
Change-Id: I308119884c42173cfef3b23c360a842c7f516977