wchevreuil commented on code in PR #5829:
URL: https://github.com/apache/hbase/pull/5829#discussion_r1570904381
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DataTieringManager.java:
##########
@@ -219,4 +225,43 @@ private long getDataTieringHotDataAge(Configuration conf) {
return Long.parseLong(
conf.get(DATATIERING_HOT_DATA_AGE_KEY,
String.valueOf(DEFAULT_DATATIERING_HOT_DATA_AGE)));
}
+
+ /*
+ * This API takes the names of files as input and returns a subset of these
file names
+ * that are cold.
+ * @parameter inputFileNames: Input list of file names
+ * @return List of names of files that are cold as per data-tiering logic.
+ */
+ public Map<String, String> getColdFilesList() {
+ Map<String, String> coldFiles = new HashMap<>();
+ for (HRegion r : this.onlineRegions.values()) {
+ for (HStore hStore : r.getStores()) {
+ Configuration conf = hStore.getReadOnlyConfiguration();
+ if (getDataTieringType(conf) != DataTieringType.TIME_RANGE) {
+ // Data-Tiering not enabled for the store. Just skip it.
+ continue;
+ }
+ Long hotDataAge = getDataTieringHotDataAge(conf);
+
+ for (HStoreFile hStoreFile : hStore.getStorefiles()) {
+ String hFileName =
+
hStoreFile.getFileInfo().getHFileInfo().getHFileContext().getHFileName();
+ OptionalLong maxTimestamp = hStoreFile.getMaximumTimestamp();
+ if (!maxTimestamp.isPresent()) {
+ // We could throw from here, But we are already in the critical
code-path
+ // of freeing space. Hence, we can ignore that file for now
+ // Or do we want to include it?
Review Comment:
That's fine for me. Can we add a debug/trace log?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -972,6 +976,13 @@ void freeSpace(final String why) {
long bytesToFreeWithExtra =
(long) Math.floor(bytesToFreeWithoutExtra * (1 + extraFreeFactor));
+ // Check the list of files to determine the cold files which can be
readily evicted.
+ Map<String, String> coldFiles =
+ DataTieringManager.getInstance().getColdFilesList();
+ //Can we evict the blocks inline during backingMap traversal?
+ // If not, we accumulate the keys and evict them later.
+ // List<BlockCacheKey> coldBlocks = new ArrayList<>();
+
Review Comment:
nit: please remove commented code.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -980,9 +991,18 @@ void freeSpace(final String why) {
BucketEntryGroup bucketMemory =
new BucketEntryGroup(bytesToFreeWithExtra, blockSize,
getPartitionSize(memoryFactor));
+ long bytesFreed = 0;
+
// Scan entire map putting bucket entry into appropriate bucket entry
// group
for (Map.Entry<BlockCacheKey, BucketEntry> bucketEntryWithKey :
backingMap.entrySet()) {
+
+ if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName()))
{
+ //coldBlocks.add(bucketEntryWithKey.getKey());
Review Comment:
nit: remove commented code.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -980,9 +991,18 @@ void freeSpace(final String why) {
BucketEntryGroup bucketMemory =
new BucketEntryGroup(bytesToFreeWithExtra, blockSize,
getPartitionSize(memoryFactor));
+ long bytesFreed = 0;
+
// Scan entire map putting bucket entry into appropriate bucket entry
// group
for (Map.Entry<BlockCacheKey, BucketEntry> bucketEntryWithKey :
backingMap.entrySet()) {
+
+ if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName()))
{
+ //coldBlocks.add(bucketEntryWithKey.getKey());
+ bytesFreed += bucketEntryWithKey.getValue().getLength();
Review Comment:
We should only account for this once the eviction was indeed successfull.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -935,6 +937,7 @@ void freeSpace(final String why) {
}
try {
freeInProgress = true;
+
Review Comment:
nit: please avoid unnecessary new lines.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -980,9 +991,18 @@ void freeSpace(final String why) {
BucketEntryGroup bucketMemory =
new BucketEntryGroup(bytesToFreeWithExtra, blockSize,
getPartitionSize(memoryFactor));
+ long bytesFreed = 0;
+
// Scan entire map putting bucket entry into appropriate bucket entry
// group
for (Map.Entry<BlockCacheKey, BucketEntry> bucketEntryWithKey :
backingMap.entrySet()) {
+
+ if (coldFiles.containsKey(bucketEntryWithKey.getKey().getHfileName()))
{
+ //coldBlocks.add(bucketEntryWithKey.getKey());
+ bytesFreed += bucketEntryWithKey.getValue().getLength();
+ evictBlock(bucketEntryWithKey.getKey());
Review Comment:
We are on an eviction process, this call would miss the proper stats
counting. Also, we should care about ref counts, so better use
`evictBucketEntryIfNoRpcReferenced` for this eviction.
Also here we are evicting all blocks for files considered cold. Shouldn't we
do it only until we reach the `bytesToFreeWithoutExtra`mark?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -980,9 +991,18 @@ void freeSpace(final String why) {
BucketEntryGroup bucketMemory =
new BucketEntryGroup(bytesToFreeWithExtra, blockSize,
getPartitionSize(memoryFactor));
+ long bytesFreed = 0;
+
Review Comment:
nit: remove new line.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
