wchevreuil commented on code in PR #5856:
URL: https://github.com/apache/hbase/pull/5856#discussion_r1582860755
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:
##########
@@ -534,7 +534,9 @@ public HRegionServer(final Configuration conf) throws
IOException {
regionServerAccounting = new RegionServerAccounting(conf);
blockCache = BlockCacheFactory.createBlockCache(conf);
- DataTieringManager.instantiate(onlineRegions);
+ if (DataTieringManager.isDataTieringFeatureEnabled(conf)) {
+ DataTieringManager.instantiate(conf, onlineRegions);
+ }
Review 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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -2195,17 +2194,19 @@ public Optional<Boolean>
blockFitsIntoTheCache(HFileBlock block) {
@Override
public Optional<Boolean> shouldCacheFile(HFileInfo hFileInfo, Configuration
conf) {
String fileName = hFileInfo.getHFileContext().getHFileName();
- try {
- DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ if (dataTieringManager != null) {
if (!dataTieringManager.isHotData(hFileInfo, conf)) {
LOG.debug("Data tiering is enabled for file: '{}' and it is not hot
data", fileName);
return Optional.of(false);
+ } else {
+ LOG.debug("Data tiering is enabled for file: '{}' and it is hot data",
fileName);
}
- } catch (IllegalStateException e) {
- LOG.error("Error while getting DataTieringManager instance: {}",
e.getMessage());
+ } else {
+ LOG.debug("Data tiering feature is not enabled. "
+ + " The file: '{}' will be loaded if not already loaded", fileName);
Review Comment:
Nit: no need for this extra else block, just do the logging.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -2195,17 +2194,19 @@ public Optional<Boolean>
blockFitsIntoTheCache(HFileBlock block) {
@Override
public Optional<Boolean> shouldCacheFile(HFileInfo hFileInfo, Configuration
conf) {
String fileName = hFileInfo.getHFileContext().getHFileName();
- try {
- DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ if (dataTieringManager != null) {
if (!dataTieringManager.isHotData(hFileInfo, conf)) {
LOG.debug("Data tiering is enabled for file: '{}' and it is not hot
data", fileName);
return Optional.of(false);
+ } else {
+ LOG.debug("Data tiering is enabled for file: '{}' and it is hot data",
fileName);
}
- } catch (IllegalStateException e) {
- LOG.error("Error while getting DataTieringManager instance: {}",
e.getMessage());
+ } else {
+ LOG.debug("Data tiering feature is not enabled. "
+ + " The file: '{}' will be loaded if not already loaded", fileName);
}
-
- // 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.
Review Comment:
Nit: avoid useless line changes in the PR.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java:
##########
@@ -40,8 +41,13 @@ public HFilePreadReader(ReaderContext context, HFileInfo
fileInfo, CacheConfig c
final MutableBoolean shouldCache = new MutableBoolean(true);
- // Initialize HFileInfo object with metadata for caching decisions
- fileInfo.initMetaAndIndex(this);
+ DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ if (dataTieringManager != null) {
+ // Initialize HFileInfo object with metadata for caching decisions.
+ // Initialize the metadata only if the data-tiering is enabled.
+ // If not, the metadata will be initialized later.
+ fileInfo.initMetaAndIndex(this);
+ }
Review 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.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDataTieringManager.java:
##########
@@ -263,47 +264,56 @@ public void testPickColdDataFiles() {
@Test
public void testBlockEvictions() throws Exception {
long capacitySize = 40 * 1024;
- int writeThreads = 3;
- int writerQLen = 64;
+ int writeThreads = 1;
+ int writerQLen = 4;
int[] bucketSizes = new int[] { 8 * 1024 + 1024 };
- // Setup: Create a bucket cache with lower capacity
- BucketCache bucketCache = new BucketCache("file:" + testDir +
"/bucket.cache", capacitySize,
- 8192, bucketSizes, writeThreads, writerQLen, testDir +
"/bucket.persistence",
- DEFAULT_ERROR_TOLERATION_DURATION, defaultConf);
-
- // Create three Cache keys with cold data files and a block with hot data.
- // hStoreFiles.get(3) is a cold data file, while hStoreFiles.get(0) is a
hot file.
- Set<BlockCacheKey> cacheKeys = new HashSet<>();
- cacheKeys.add(new BlockCacheKey(hStoreFiles.get(3).getPath(), 0, true,
BlockType.DATA));
- cacheKeys.add(new BlockCacheKey(hStoreFiles.get(3).getPath(), 8192, true,
BlockType.DATA));
- cacheKeys.add(new BlockCacheKey(hStoreFiles.get(0).getPath(), 0, true,
BlockType.DATA));
-
- // Create dummy data to be cached and fill the cache completely.
- CacheTestUtils.HFileBlockPair[] blocks =
CacheTestUtils.generateHFileBlocks(8192, 3);
-
- int blocksIter = 0;
- for (BlockCacheKey key : cacheKeys) {
- bucketCache.cacheBlock(key, blocks[blocksIter++].getBlock());
- // Ensure that the block is persisted to the file.
- 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);
Review Comment:
Is this necessary because of the modified functionality? Otherwise, can you
explain why this is needed now?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DataTieringManager.java:
##########
@@ -61,25 +63,25 @@ private DataTieringManager(Map<String, HRegion>
onlineRegions) {
* Initializes the DataTieringManager instance with the provided map of
online regions.
* @param onlineRegions A map containing online regions.
*/
- public static synchronized void instantiate(Map<String, HRegion>
onlineRegions) {
- if (instance == null) {
- instance = new DataTieringManager(onlineRegions);
- LOG.info("DataTieringManager instantiated successfully.");
+ public static synchronized void instantiate(Configuration conf,
Review 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`.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -2195,17 +2194,19 @@ public Optional<Boolean>
blockFitsIntoTheCache(HFileBlock block) {
@Override
public Optional<Boolean> shouldCacheFile(HFileInfo hFileInfo, Configuration
conf) {
String fileName = hFileInfo.getHFileContext().getHFileName();
- try {
- DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ if (dataTieringManager != null) {
if (!dataTieringManager.isHotData(hFileInfo, conf)) {
LOG.debug("Data tiering is enabled for file: '{}' and it is not hot
data", fileName);
return Optional.of(false);
+ } else {
+ LOG.debug("Data tiering is enabled for file: '{}' and it is hot data",
fileName);
Review Comment:
Nit: no need for this extra else block, just do the logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
