From 96b7fe3f5ec589b476e2a603d4eaf57e80b54e5f Mon Sep 17 00:00:00 2001
From: "terence.yoo" <terence.yoo@kakaocorp.com>
Date: Thu, 29 Dec 2022 18:44:58 +0900
Subject: [PATCH] Close store file readers after region warmup

---
 .../hadoop/hbase/regionserver/HStoreFile.java | 14 +++++++++-
 .../hbase/regionserver/StoreEngine.java       |  8 ++++--
 .../hbase/regionserver/TestHStoreFile.java    | 27 +++++++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreFile.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreFile.java
index ae514f0aef8d..3e39c2ef6e9b 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreFile.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreFile.java
@@ -378,6 +378,10 @@ public HDFSBlocksDistribution getHDFSBlockDistribution() {
    * @see #closeStoreFile(boolean)
    */
   private void open() throws IOException {
+    open(false);
+  }
+
+  private void open(boolean warmup) throws IOException {
     fileInfo.initHDFSBlocksDistribution();
     long readahead = fileInfo.isNoReadahead() ? 0L : -1L;
     ReaderContext context = fileInfo.createReaderContext(false, readahead, ReaderType.PREAD);
@@ -499,17 +503,25 @@ private void open() throws IOException {
     firstKey = initialReader.getFirstKey();
     lastKey = initialReader.getLastKey();
     comparator = initialReader.getComparator();
+
+    if (warmup) {
+      closeStoreFile(cacheConf == null || cacheConf.shouldEvictOnClose());
+    }
   }
 
   /**
    * Initialize the reader used for pread.
    */
   public void initReader() throws IOException {
+    initReader(false);
+  }
+
+  public void initReader(boolean warmup) throws IOException {
     if (initialReader == null) {
       synchronized (this) {
         if (initialReader == null) {
           try {
-            open();
+            open(warmup);
           } catch (Exception e) {
             try {
               boolean evictOnClose = cacheConf != null ? cacheConf.shouldEvictOnClose() : true;
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java
index f6e3db0116bb..339349b2ad68 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java
@@ -219,10 +219,14 @@ public HStoreFile createStoreFileAndReader(Path p) throws IOException {
   }
 
   public HStoreFile createStoreFileAndReader(StoreFileInfo info) throws IOException {
+    return createStoreFileAndReader(info, false);
+  }
+
+  public HStoreFile createStoreFileAndReader(StoreFileInfo info, boolean warmup) throws IOException {
     info.setRegionCoprocessorHost(coprocessorHost);
     HStoreFile storeFile = new HStoreFile(info, ctx.getFamily().getBloomFilterType(),
       ctx.getCacheConf(), bloomFilterMetrics);
-    storeFile.initReader();
+    storeFile.initReader(warmup);
     return storeFile;
   }
 
@@ -263,7 +267,7 @@ private List<HStoreFile> openStoreFiles(Collection<StoreFileInfo> files, boolean
       // our store's CompoundConfiguration here.
       storeFileInfo.setConf(conf);
       // open each store file in parallel
-      completionService.submit(() -> createStoreFileAndReader(storeFileInfo));
+      completionService.submit(() -> createStoreFileAndReader(storeFileInfo, warmup));
       totalValidStoreFile++;
     }
 
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStoreFile.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStoreFile.java
index a0c23af5ef0d..cc23491bf7b3 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStoreFile.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStoreFile.java
@@ -1279,4 +1279,31 @@ private void testDataBlockSizeWithCompressionRatePredicator(int expectedBlockCou
     assertEquals(expectedBlockCount, blockCount);
   }
 
+  @Test
+  public void testInitReaderForWarmup() throws Exception {
+    final RegionInfo hri =
+      RegionInfoBuilder.newBuilder(TableName.valueOf("testInitReaderForWarmup")).build();
+    HRegionFileSystem regionFs = HRegionFileSystem.createRegionOnFileSystem(conf, fs,
+      new Path(testDir, hri.getTable().getNameAsString()), hri);
+    HFileContext meta = new HFileContextBuilder().withBlockSize(8 * 1024).build();
+
+    // Make a store file and write data to it.
+    StoreFileWriter writer = new StoreFileWriter.Builder(conf, cacheConf, this.fs)
+      .withFilePath(regionFs.createTempName()).withFileContext(meta).build();
+    writeStoreFile(writer);
+    Path hsfPath = regionFs.commitStoreFile(TEST_FAMILY, writer.getPath());
+    writer.close();
+
+    HStoreFile file = Mockito.spy(new HStoreFile(this.fs, hsfPath, conf, cacheConf, BloomType.NONE, true));
+
+    // after warmup the file reader should be closed and null to avoid file descriptor leakage
+    file.initReader(true);
+    assertNull(file.getReader());
+    Mockito.verify(file, Mockito.times(1)).closeStoreFile(Mockito.anyBoolean());
+
+    // not for warmup
+    file.initReader();
+    assertNotNull(file.getReader());
+    Mockito.verify(file, Mockito.times(1)).closeStoreFile(Mockito.anyBoolean());
+  }
 }