Skip to content

Commit 290ae1e

Browse files
wchevreuilwchevreuil
authored
HBASE-29707 Fix region cache % metrics miss calculation (#7451)
Signed-off-by: Peter Somogyi <psomogyi@apache.org> Reviewed-by: Kevin Geiszler <kevin.j.geiszler@gmail.com>
1 parent c9ecf37 commit 290ae1e

File tree

8 files changed

+205
-28
lines changed

8 files changed

+205
-28
lines changed

‎hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,14 @@ default Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repe
100100
*/
101101
int evictBlocksByHfileName(String hfileName);
102102

103+
/**
104+
* Evicts all blocks for the given HFile by path.
105+
* @return the number of blocks evicted
106+
*/
107+
default int evictBlocksByHfilePath(Path hfilePath) {
108+
return evictBlocksByHfileName(hfilePath.getName());
109+
}
110+
103111
/**
104112
* Get the statistics for this block cache.
105113
*/

‎hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheKey.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ public class BlockCacheKey implements HeapSize, java.io.Serializable {
3232
private final long offset;
3333
private BlockType blockType;
3434
private final boolean isPrimaryReplicaBlock;
35+
3536
private Path filePath;
3637

3738
/**
@@ -116,4 +117,9 @@ public void setBlockType(BlockType blockType) {
116117
public Path getFilePath() {
117118
return filePath;
118119
}
120+
121+
public void setFilePath(Path filePath) {
122+
this.filePath = filePath;
123+
}
124+
119125
}

‎hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,11 @@ public int evictBlocksByHfileName(String hfileName) {
159159
return l1Cache.evictBlocksByHfileName(hfileName) + l2Cache.evictBlocksByHfileName(hfileName);
160160
}
161161

162+
@Override
163+
public int evictBlocksByHfilePath(Path hfilePath) {
164+
return l1Cache.evictBlocksByHfilePath(hfilePath) + l2Cache.evictBlocksByHfilePath(hfilePath);
165+
}
166+
162167
@Override
163168
public CacheStats getStats() {
164169
return this.combinedCacheStats;

‎hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public void close(boolean evictOnClose) throws IOException {
185185
// Deallocate data blocks
186186
cacheConf.getBlockCache().ifPresent(cache -> {
187187
if (evictOnClose) {
188-
int numEvicted = cache.evictBlocksByHfileName(name);
188+
int numEvicted = cache.evictBlocksByHfilePath(path);
189189
if (LOG.isTraceEnabled()) {
190190
LOG.trace("On close, file= {} evicted= {} block(s)", name, numEvicted);
191191
}

‎hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java‎

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
import org.apache.hadoop.hbase.regionserver.StoreFileInfo;
8989
import org.apache.hadoop.hbase.util.Bytes;
9090
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
91+
import org.apache.hadoop.hbase.util.HFileArchiveUtil;
9192
import org.apache.hadoop.hbase.util.IdReadWriteLock;
9293
import org.apache.hadoop.hbase.util.IdReadWriteLockStrongRef;
9394
import org.apache.hadoop.hbase.util.IdReadWriteLockWithObjectPool;
@@ -722,7 +723,7 @@ public Cacheable getBlock(BlockCacheKey key, boolean caching, boolean repeat,
722723
// the cache map state might differ from the actual cache. If we reach this block,
723724
// we should remove the cache key entry from the backing map
724725
backingMap.remove(key);
725-
fullyCachedFiles.remove(key.getHfileName());
726+
fileNotFullyCached(key, bucketEntry);
726727
LOG.debug("Failed to fetch block for cache key: {}.", key, hioex);
727728
} catch (IOException ioex) {
728729
LOG.error("Failed reading block " + key + " from bucket cache", ioex);
@@ -747,7 +748,7 @@ void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry, boolean decre
747748
if (decrementBlockNumber) {
748749
this.blockNumber.decrement();
749750
if (ioEngine.isPersistent()) {
750-
fileNotFullyCached(cacheKey.getHfileName());
751+
fileNotFullyCached(cacheKey, bucketEntry);
751752
}
752753
}
753754
if (evictedByEvictionProcess) {
@@ -758,23 +759,11 @@ void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry, boolean decre
758759
}
759760
}
760761

761-
private void fileNotFullyCached(String hfileName) {
762-
// Update the regionPrefetchedSizeMap before removing the file from prefetchCompleted
763-
if (fullyCachedFiles.containsKey(hfileName)) {
764-
Pair<String, Long> regionEntry = fullyCachedFiles.get(hfileName);
765-
String regionEncodedName = regionEntry.getFirst();
766-
long filePrefetchSize = regionEntry.getSecond();
767-
LOG.debug("Removing file {} for region {}", hfileName, regionEncodedName);
768-
regionCachedSize.computeIfPresent(regionEncodedName, (rn, pf) -> pf - filePrefetchSize);
769-
// If all the blocks for a region are evicted from the cache, remove the entry for that region
770-
if (
771-
regionCachedSize.containsKey(regionEncodedName)
772-
&& regionCachedSize.get(regionEncodedName) == 0
773-
) {
774-
regionCachedSize.remove(regionEncodedName);
775-
}
776-
}
777-
fullyCachedFiles.remove(hfileName);
762+
private void fileNotFullyCached(BlockCacheKey key, BucketEntry entry) {
763+
// Update the updateRegionCachedSize before removing the file from fullyCachedFiles.
764+
// This computation should happen even if the file is not in fullyCachedFiles map.
765+
updateRegionCachedSize(key.getFilePath(), (entry.getLength() * -1));
766+
fullyCachedFiles.remove(key.getHfileName());
778767
}
779768

780769
public void fileCacheCompleted(Path filePath, long size) {
@@ -788,9 +777,19 @@ public void fileCacheCompleted(Path filePath, long size) {
788777

789778
private void updateRegionCachedSize(Path filePath, long cachedSize) {
790779
if (filePath != null) {
791-
String regionName = filePath.getParent().getParent().getName();
792-
regionCachedSize.merge(regionName, cachedSize,
793-
(previousSize, newBlockSize) -> previousSize + newBlockSize);
780+
if (HFileArchiveUtil.isHFileArchived(filePath)) {
781+
LOG.trace("Skipping region cached size update for archived file: {}", filePath);
782+
} else {
783+
String regionName = filePath.getParent().getParent().getName();
784+
regionCachedSize.merge(regionName, cachedSize,
785+
(previousSize, newBlockSize) -> previousSize + newBlockSize);
786+
LOG.trace("Updating region cached size for region: {}", regionName);
787+
// If all the blocks for a region are evicted from the cache,
788+
// remove the entry for that region from regionCachedSize map.
789+
if (regionCachedSize.get(regionName) <= 0) {
790+
regionCachedSize.remove(regionName);
791+
}
792+
}
794793
}
795794
}
796795

@@ -1698,7 +1697,7 @@ private void verifyFileIntegrity(BucketCacheProtos.BucketCacheEntry proto) {
16981697
} catch (IOException e1) {
16991698
LOG.debug("Check for key {} failed. Evicting.", keyEntry.getKey());
17001699
evictBlock(keyEntry.getKey());
1701-
fileNotFullyCached(keyEntry.getKey().getHfileName());
1700+
fileNotFullyCached(keyEntry.getKey(), keyEntry.getValue());
17021701
}
17031702
}
17041703
backingMapValidated.set(true);
@@ -1928,20 +1927,32 @@ public int evictBlocksByHfileName(String hfileName) {
19281927
}
19291928

19301929
@Override
1931-
public int evictBlocksRangeByHfileName(String hfileName, long initOffset, long endOffset) {
1932-
fileNotFullyCached(hfileName);
1930+
public int evictBlocksByHfilePath(Path hfilePath) {
1931+
return evictBlocksRangeByHfileName(hfilePath.getName(), hfilePath, 0, Long.MAX_VALUE);
1932+
}
1933+
1934+
public int evictBlocksRangeByHfileName(String hfileName, Path filePath, long initOffset,
1935+
long endOffset) {
19331936
Set<BlockCacheKey> keySet = getAllCacheKeysForFile(hfileName, initOffset, endOffset);
19341937
LOG.debug("found {} blocks for file {}, starting offset: {}, end offset: {}", keySet.size(),
19351938
hfileName, initOffset, endOffset);
19361939
int numEvicted = 0;
19371940
for (BlockCacheKey key : keySet) {
1941+
if (filePath != null) {
1942+
key.setFilePath(filePath);
1943+
}
19381944
if (evictBlock(key)) {
19391945
++numEvicted;
19401946
}
19411947
}
19421948
return numEvicted;
19431949
}
19441950

1951+
@Override
1952+
public int evictBlocksRangeByHfileName(String hfileName, long initOffset, long endOffset) {
1953+
return evictBlocksRangeByHfileName(hfileName, null, initOffset, endOffset);
1954+
}
1955+
19451956
private Set<BlockCacheKey> getAllCacheKeysForFile(String hfileName, long init, long end) {
19461957
return blocksByHFile.subSet(new BlockCacheKey(hfileName, init), true,
19471958
new BlockCacheKey(hfileName, end), true);

‎hbase-server/src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,4 +200,16 @@ public static TableName getTableName(Path archivePath) {
200200
if (p == null) return null;
201201
return TableName.valueOf(p.getName(), tbl);
202202
}
203+
204+
public static boolean isHFileArchived(Path path) {
205+
Path currentDir = path;
206+
for (int i = 0; i < 6; i++) {
207+
currentDir = currentDir.getParent();
208+
if (currentDir == null) {
209+
return false;
210+
}
211+
}
212+
return HConstants.HFILE_ARCHIVE_DIRECTORY.equals(currentDir.getName());
213+
}
214+
203215
}

‎hbase-server/src/test/java/org/apache/hadoop/hbase/TestCacheEviction.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.apache.hadoop.hbase.testclassification.MiscTests;
4343
import org.apache.hadoop.hbase.util.Bytes;
4444
import org.apache.hadoop.hbase.util.Pair;
45+
import org.junit.Before;
4546
import org.junit.BeforeClass;
4647
import org.junit.ClassRule;
4748
import org.junit.Test;
@@ -66,11 +67,16 @@ public static void setUp() throws Exception {
6667
UTIL.getConfiguration().setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, 2);
6768
UTIL.getConfiguration().setBoolean(CACHE_BLOCKS_ON_WRITE_KEY, true);
6869
UTIL.getConfiguration().setBoolean(PREFETCH_BLOCKS_ON_OPEN_KEY, true);
69-
UTIL.getConfiguration().set(BUCKET_CACHE_IOENGINE_KEY, "offheap");
7070
UTIL.getConfiguration().setInt(BUCKET_CACHE_SIZE_KEY, 200);
7171
UTIL.getConfiguration().set(StoreFileTrackerFactory.TRACKER_IMPL, "FILE");
7272
}
7373

74+
@Before
75+
public void testSetup() {
76+
UTIL.getConfiguration().set(BUCKET_CACHE_IOENGINE_KEY,
77+
"file:" + UTIL.getDataTestDir() + "/bucketcache");
78+
}
79+
7480
@Test
7581
public void testEvictOnSplit() throws Exception {
7682
doTestEvictOnSplit("testEvictOnSplit", true,

‎hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestPrefetchWithBucketCache.java‎

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import org.apache.hadoop.hbase.client.RegionInfoBuilder;
5252
import org.apache.hadoop.hbase.fs.HFileSystem;
5353
import org.apache.hadoop.hbase.io.ByteBuffAllocator;
54+
import org.apache.hadoop.hbase.io.HFileLink;
5455
import org.apache.hadoop.hbase.io.hfile.BlockCache;
5556
import org.apache.hadoop.hbase.io.hfile.BlockCacheFactory;
5657
import org.apache.hadoop.hbase.io.hfile.BlockCacheKey;
@@ -71,12 +72,14 @@
7172
import org.apache.hadoop.hbase.regionserver.HRegionFileSystem;
7273
import org.apache.hadoop.hbase.regionserver.HStoreFile;
7374
import org.apache.hadoop.hbase.regionserver.StoreContext;
75+
import org.apache.hadoop.hbase.regionserver.StoreFileInfo;
7476
import org.apache.hadoop.hbase.regionserver.StoreFileWriter;
7577
import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTracker;
7678
import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory;
7779
import org.apache.hadoop.hbase.testclassification.IOTests;
7880
import org.apache.hadoop.hbase.testclassification.MediumTests;
7981
import org.apache.hadoop.hbase.util.Bytes;
82+
import org.apache.hadoop.hbase.util.CommonFSUtils;
8083
import org.junit.After;
8184
import org.junit.Before;
8285
import org.junit.ClassRule;
@@ -363,7 +366,7 @@ public void testPrefetchMetricProgress() throws Exception {
363366
BucketCache bc = BucketCache.getBucketCacheFromCacheConfig(cacheConf).get();
364367
MutableLong regionCachedSize = new MutableLong(0);
365368
// Our file should have 6 DATA blocks. We should wait for all of them to be cached
366-
long waitedTime = Waiter.waitFor(conf, 300, () -> {
369+
Waiter.waitFor(conf, 300, () -> {
367370
if (bc.getBackingMap().size() > 0) {
368371
long currentSize = bc.getRegionCachedInfo().get().get(regionName);
369372
assertTrue(regionCachedSize.getValue() <= currentSize);
@@ -374,6 +377,132 @@ public void testPrefetchMetricProgress() throws Exception {
374377
});
375378
}
376379

380+
@Test
381+
public void testPrefetchMetricProgressForLinks() throws Exception {
382+
conf.setLong(BUCKET_CACHE_SIZE_KEY, 200);
383+
blockCache = BlockCacheFactory.createBlockCache(conf);
384+
cacheConf = new CacheConfig(conf, blockCache);
385+
final RegionInfo hri =
386+
RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName())).build();
387+
// force temp data in hbase/target/test-data instead of /tmp/hbase-xxxx/
388+
Configuration testConf = new Configuration(this.conf);
389+
Path testDir = TEST_UTIL.getDataTestDir(name.getMethodName());
390+
CommonFSUtils.setRootDir(testConf, testDir);
391+
Path tableDir = CommonFSUtils.getTableDir(testDir, hri.getTable());
392+
RegionInfo region = RegionInfoBuilder.newBuilder(TableName.valueOf(tableDir.getName())).build();
393+
Path regionDir = new Path(tableDir, region.getEncodedName());
394+
Path cfDir = new Path(regionDir, "cf");
395+
HRegionFileSystem regionFS =
396+
HRegionFileSystem.createRegionOnFileSystem(testConf, fs, tableDir, region);
397+
Path storeFile = writeStoreFile(100, cfDir);
398+
// Prefetches the file blocks
399+
LOG.debug("First read should prefetch the blocks.");
400+
readStoreFile(storeFile);
401+
BucketCache bc = BucketCache.getBucketCacheFromCacheConfig(cacheConf).get();
402+
// Our file should have 6 DATA blocks. We should wait for all of them to be cached
403+
Waiter.waitFor(testConf, 300, () -> bc.getBackingMap().size() == 6);
404+
long cachedSize = bc.getRegionCachedInfo().get().get(region.getEncodedName());
405+
406+
final RegionInfo dstHri =
407+
RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName())).build();
408+
HRegionFileSystem dstRegionFs = HRegionFileSystem.createRegionOnFileSystem(testConf, fs,
409+
CommonFSUtils.getTableDir(testDir, dstHri.getTable()), dstHri);
410+
411+
Path dstPath = new Path(regionFS.getTableDir(), new Path(dstHri.getRegionNameAsString(), "cf"));
412+
413+
Path linkFilePath =
414+
new Path(dstPath, HFileLink.createHFileLinkName(region, storeFile.getName()));
415+
416+
StoreFileTracker sft = StoreFileTrackerFactory.create(testConf, false,
417+
StoreContext.getBuilder().withFamilyStoreDirectoryPath(dstPath)
418+
.withColumnFamilyDescriptor(ColumnFamilyDescriptorBuilder.of("cf"))
419+
.withRegionFileSystem(dstRegionFs).build());
420+
sft.createHFileLink(hri.getTable(), hri.getEncodedName(), storeFile.getName(), true);
421+
StoreFileInfo sfi = sft.getStoreFileInfo(linkFilePath, true);
422+
423+
HStoreFile hsf = new HStoreFile(sfi, BloomType.NONE, cacheConf);
424+
assertTrue(sfi.isLink());
425+
hsf.initReader();
426+
HFile.Reader reader = hsf.getReader().getHFileReader();
427+
while (!reader.prefetchComplete()) {
428+
// Sleep for a bit
429+
Thread.sleep(1000);
430+
}
431+
// HFileLink use the path of the target file to create a reader, so it should resolve to the
432+
// already cached blocks and not insert new blocks in the cache.
433+
Waiter.waitFor(testConf, 300, () -> bc.getBackingMap().size() == 6);
434+
435+
assertEquals(cachedSize, (long) bc.getRegionCachedInfo().get().get(region.getEncodedName()));
436+
}
437+
438+
@Test
439+
public void testPrefetchMetricProgressForLinksToArchived() throws Exception {
440+
conf.setLong(BUCKET_CACHE_SIZE_KEY, 200);
441+
blockCache = BlockCacheFactory.createBlockCache(conf);
442+
cacheConf = new CacheConfig(conf, blockCache);
443+
444+
// force temp data in hbase/target/test-data instead of /tmp/hbase-xxxx/
445+
Configuration testConf = new Configuration(this.conf);
446+
Path testDir = TEST_UTIL.getDataTestDir(name.getMethodName());
447+
CommonFSUtils.setRootDir(testConf, testDir);
448+
449+
final RegionInfo hri =
450+
RegionInfoBuilder.newBuilder(TableName.valueOf(name.getMethodName())).build();
451+
Path tableDir = CommonFSUtils.getTableDir(testDir, hri.getTable());
452+
RegionInfo region = RegionInfoBuilder.newBuilder(TableName.valueOf(tableDir.getName())).build();
453+
Path regionDir = new Path(tableDir, region.getEncodedName());
454+
Path cfDir = new Path(regionDir, "cf");
455+
456+
Path storeFile = writeStoreFile(100, cfDir);
457+
// Prefetches the file blocks
458+
LOG.debug("First read should prefetch the blocks.");
459+
readStoreFile(storeFile);
460+
BucketCache bc = BucketCache.getBucketCacheFromCacheConfig(cacheConf).get();
461+
// Our file should have 6 DATA blocks. We should wait for all of them to be cached
462+
Waiter.waitFor(testConf, 300, () -> bc.getBackingMap().size() == 6);
463+
long cachedSize = bc.getRegionCachedInfo().get().get(region.getEncodedName());
464+
465+
// create another file, but in the archive dir, hence it won't be cached
466+
Path archiveRoot = new Path(testDir, "archive");
467+
Path archiveTableDir = CommonFSUtils.getTableDir(archiveRoot, hri.getTable());
468+
Path archiveRegionDir = new Path(archiveTableDir, region.getEncodedName());
469+
Path archiveCfDir = new Path(archiveRegionDir, "cf");
470+
Path archivedFile = writeStoreFile(100, archiveCfDir);
471+
472+
final RegionInfo testRegion =
473+
RegionInfoBuilder.newBuilder(TableName.valueOf(tableDir.getName())).build();
474+
final HRegionFileSystem testRegionFs = HRegionFileSystem.createRegionOnFileSystem(testConf, fs,
475+
CommonFSUtils.getTableDir(testDir, testRegion.getTable()), testRegion);
476+
// Just create a link to the archived file
477+
Path dstPath = new Path(tableDir, new Path(testRegion.getEncodedName(), "cf"));
478+
479+
Path linkFilePath =
480+
new Path(dstPath, HFileLink.createHFileLinkName(region, archivedFile.getName()));
481+
482+
StoreFileTracker sft = StoreFileTrackerFactory.create(testConf, false,
483+
StoreContext.getBuilder().withFamilyStoreDirectoryPath(dstPath)
484+
.withColumnFamilyDescriptor(ColumnFamilyDescriptorBuilder.of("cf"))
485+
.withRegionFileSystem(testRegionFs).build());
486+
sft.createHFileLink(hri.getTable(), hri.getEncodedName(), storeFile.getName(), true);
487+
StoreFileInfo sfi = sft.getStoreFileInfo(linkFilePath, true);
488+
489+
HStoreFile hsf = new HStoreFile(sfi, BloomType.NONE, cacheConf);
490+
assertTrue(sfi.isLink());
491+
hsf.initReader();
492+
HFile.Reader reader = hsf.getReader().getHFileReader();
493+
while (!reader.prefetchComplete()) {
494+
// Sleep for a bit
495+
Thread.sleep(1000);
496+
}
497+
// HFileLink use the path of the target file to create a reader, but the target file is in the
498+
// archive, so it wasn't cached previously and should be cached when we open the link.
499+
Waiter.waitFor(testConf, 300, () -> bc.getBackingMap().size() == 12);
500+
// cached size for the region of target file shouldn't change
501+
assertEquals(cachedSize, (long) bc.getRegionCachedInfo().get().get(region.getEncodedName()));
502+
// cached size for the region with link pointing to archive dir shouldn't be updated
503+
assertNull(bc.getRegionCachedInfo().get().get(testRegion.getEncodedName()));
504+
}
505+
377506
private void readStoreFile(Path storeFilePath) throws Exception {
378507
readStoreFile(storeFilePath, (r, o) -> {
379508
HFileBlock block = null;

0 commit comments

Comments
 (0)