jhungund commented on code in PR #5866:
URL: https://github.com/apache/hbase/pull/5866#discussion_r1601814522
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -2203,6 +2204,21 @@ public Optional<Boolean> shouldCacheFile(HFileInfo
hFileInfo, Configuration conf
return Optional.of(!fullyCachedFiles.containsKey(fileName));
}
+ @Override
+ public Optional<Boolean> shouldCacheBlock(BlockCacheKey key, Configuration
conf) {
+ try {
+ DataTieringManager dataTieringManager = DataTieringManager.getInstance();
+ if (dataTieringManager != null && !dataTieringManager.isHotData(key,
conf)) {
Review Comment:
Hi @wchevreuil, In my humble opinion, we usually code with the following
order of priorities:
Functional correctness > Performance/Resource utilisation > Coding
guidelines/principles
We cannot let go of functional correctness for the sake of performance or
for coding principles.
Similarly, we may not want to relax the performance/resource utilisation for
the sake of adhering to coding principles.
Here we are repurposing an object that stays in memory forever as an
information carrier for our utility function (isHotData). Once the utility
function finishes, we will never use that data again.
Can we have a utility defined in DataTieringManager to take timestamps,
configuration objects and make decisions for us?
Sorry, I do not want to hold on to this change by being inflexible, but
trying to explain that we can achieve the same result by relaxing the OO coding
principles instead of relaxing the memory utilisation.
@vinayakphegde and I will spend one more day and try to come up with a
solution. If we can't come up with a solution, we can go ahead and submit this
change.
Thanks,
Janardhan
--
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]
