HBASE-28527: Adjust BlockCacheKey to use the file path instead of file name. #5832
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,22 +25,28 @@ | |
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentSkipListSet; | ||
| import java.util.function.Function; | ||
| import org.apache.hadoop.fs.Path; | ||
| import org.apache.hadoop.hbase.io.ByteBuffAllocator; | ||
| import org.apache.hadoop.hbase.io.ByteBuffAllocator.Recycler; | ||
| import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; | ||
| import org.apache.hadoop.hbase.io.hfile.BlockPriority; | ||
| import org.apache.hadoop.hbase.io.hfile.BlockType; | ||
| import org.apache.hadoop.hbase.io.hfile.CacheableDeserializerIdManager; | ||
| import org.apache.hadoop.hbase.io.hfile.HFileBlock; | ||
| import org.apache.hadoop.hbase.regionserver.DataTieringManager; | ||
| import org.apache.hadoop.hbase.util.Pair; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| import org.apache.hbase.thirdparty.com.google.protobuf.ByteString; | ||
|
|
||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.BucketCacheProtos; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @InterfaceAudience.Private | ||
| final class BucketProtoUtils { | ||
| private static final Logger LOG = LoggerFactory.getLogger(BucketProtoUtils.class); | ||
|
|
||
| private BucketProtoUtils() { | ||
|
|
||
| } | ||
|
|
@@ -130,10 +136,30 @@ static Pair<ConcurrentHashMap<BlockCacheKey, BucketEntry>, NavigableSet<BlockCac | |
| ConcurrentHashMap<BlockCacheKey, BucketEntry> result = new ConcurrentHashMap<>(); | ||
| NavigableSet<BlockCacheKey> resultSet = new ConcurrentSkipListSet<>(Comparator | ||
| .comparing(BlockCacheKey::getHfileName).thenComparingLong(BlockCacheKey::getOffset)); | ||
|
|
||
| Map<String, Path> allFilePaths = null; | ||
| DataTieringManager dataTieringManager; | ||
| try { | ||
| dataTieringManager = DataTieringManager.getInstance(); | ||
| allFilePaths = dataTieringManager.getAllFilesList(); | ||
|
There was a problem hiding this comment. We don't need this for non TIME_RANGE tiering type, right? Can we avoid it, then? |
||
| } catch (IllegalStateException e) { | ||
| // Data-Tiering manager has not been set up. | ||
| // Ignore the error and proceed with the normal flow. | ||
| LOG.warn("Error while getting DataTieringManager instance: {}", e.getMessage()); | ||
|
Comment on lines
+146
to
+148
There was a problem hiding this comment. Is this going to be thrown whenever data tiering type isn't set to TIME_RANGE? |
||
| } | ||
|
|
||
| for (BucketCacheProtos.BackingMapEntry entry : backingMap.getEntryList()) { | ||
| BucketCacheProtos.BlockCacheKey protoKey = entry.getKey(); | ||
|
|
||
| BlockCacheKey key = null; | ||
| if (allFilePaths != null) { | ||
| key = new BlockCacheKey(allFilePaths.get(protoKey.getHfilename()), protoKey.getOffset(), | ||
|
There was a problem hiding this comment.
|
||
| protoKey.getPrimaryReplicaBlock(), fromPb(protoKey.getBlockType())); | ||
| } else { | ||
| key = new BlockCacheKey(new Path(protoKey.getHfilename()), protoKey.getOffset(), | ||
|
There was a problem hiding this comment. This call will set the file name appropriately to protoKey.getHfilename(). There was a problem hiding this comment. The path will be incorrect, right? Anyone who accesses it will find the incorrect path. There was a problem hiding this comment. One way I can think of avoiding incorrect path to be set is by adding the following check in the constructor of BlockCacheKey: There was a problem hiding this comment. In which cases will And when it isn't null, what do we do with keys that weren't found in |
||
| protoKey.getPrimaryReplicaBlock(), fromPb(protoKey.getBlockType())); | ||
| } | ||
|
|
||
| BucketCacheProtos.BucketEntry protoValue = entry.getValue(); | ||
| // TODO:We use ByteBuffAllocator.HEAP here, because we could not get the ByteBuffAllocator | ||
| // which created by RpcServer elegantly. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.hadoop.hbase.regionserver; | ||
|
|
||
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| @InterfaceAudience.Private | ||
| public class DataTieringException extends Exception { | ||
| DataTieringException(String reason) { | ||
| super(reason); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just leave these and avoid having to touch every single existing test? As having a complete path is only relevant for the TIME_RANGE data tiering, I think it's fine to allow non complete paths when TIME_RANGE data tiering is not in use.