vinayakphegde commented on code in PR #5856:
URL: https://github.com/apache/hbase/pull/5856#discussion_r1582666798
##########
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:
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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DataTieringManager.java:
##########
@@ -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";
Review 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`?
##########
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:
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?
##########
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:
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?
--
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]
