Close store file readers after region warmup

apache · Dec 29, 2022 · 96b7fe3 · 96b7fe3
1 parent 7ef63b6
commit 96b7fe3
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 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
@@ -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
@@ -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
@@ -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());
+  }
 }