From 96b7fe3f5ec589b476e2a603d4eaf57e80b54e5f Mon Sep 17 00:00:00 2001 From: "terence.yoo" Date: Thu, 29 Dec 2022 18:44:58 +0900 Subject: [PATCH 1/5] 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 openStoreFiles(Collection 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()); + } } From 9c22e816426c6984f6cd9512c594b92d107872a7 Mon Sep 17 00:00:00 2001 From: "terence.yoo" Date: Thu, 29 Dec 2022 19:51:29 +0900 Subject: [PATCH 2/5] fix violations --- .../java/org/apache/hadoop/hbase/regionserver/HStoreFile.java | 4 ---- .../org/apache/hadoop/hbase/regionserver/StoreEngine.java | 3 ++- .../org/apache/hadoop/hbase/regionserver/TestHStoreFile.java | 3 ++- 3 files changed, 4 insertions(+), 6 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 3e39c2ef6e9b..73a684b5e972 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 @@ -377,10 +377,6 @@ public HDFSBlocksDistribution getHDFSBlockDistribution() { * Opens reader on this store file. Called by Constructor. * @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; 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 339349b2ad68..47473a5985d2 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 @@ -222,7 +222,8 @@ public HStoreFile createStoreFileAndReader(StoreFileInfo info) throws IOExceptio return createStoreFileAndReader(info, false); } - public HStoreFile createStoreFileAndReader(StoreFileInfo info, boolean warmup) throws IOException { + public HStoreFile createStoreFileAndReader(StoreFileInfo info, boolean warmup) + throws IOException { info.setRegionCoprocessorHost(coprocessorHost); HStoreFile storeFile = new HStoreFile(info, ctx.getFamily().getBloomFilterType(), ctx.getCacheConf(), bloomFilterMetrics); 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 cc23491bf7b3..fec008129a39 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 @@ -1294,7 +1294,8 @@ public void testInitReaderForWarmup() throws Exception { Path hsfPath = regionFs.commitStoreFile(TEST_FAMILY, writer.getPath()); writer.close(); - HStoreFile file = Mockito.spy(new HStoreFile(this.fs, hsfPath, conf, cacheConf, BloomType.NONE, true)); + 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); From 377c5e17cd478e513714931f14fb93ddb3beb669 Mon Sep 17 00:00:00 2001 From: "terence.yoo" Date: Wed, 11 Jan 2023 19:56:39 +0900 Subject: [PATCH 3/5] Revert two commits --- .../hadoop/hbase/regionserver/HStoreFile.java | 12 ++------ .../hbase/regionserver/StoreEngine.java | 9 ++---- .../hbase/regionserver/TestHStoreFile.java | 28 ------------------- 3 files changed, 4 insertions(+), 45 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 73a684b5e972..ae514f0aef8d 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 @@ -377,7 +377,7 @@ public HDFSBlocksDistribution getHDFSBlockDistribution() { * Opens reader on this store file. Called by Constructor. * @see #closeStoreFile(boolean) */ - private void open(boolean warmup) throws IOException { + private void open() throws IOException { fileInfo.initHDFSBlocksDistribution(); long readahead = fileInfo.isNoReadahead() ? 0L : -1L; ReaderContext context = fileInfo.createReaderContext(false, readahead, ReaderType.PREAD); @@ -499,25 +499,17 @@ private void open(boolean warmup) 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(warmup); + open(); } 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 47473a5985d2..f6e3db0116bb 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,15 +219,10 @@ 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(warmup); + storeFile.initReader(); return storeFile; } @@ -268,7 +263,7 @@ private List openStoreFiles(Collection files, boolean // our store's CompoundConfiguration here. storeFileInfo.setConf(conf); // open each store file in parallel - completionService.submit(() -> createStoreFileAndReader(storeFileInfo, warmup)); + completionService.submit(() -> createStoreFileAndReader(storeFileInfo)); 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 fec008129a39..a0c23af5ef0d 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,32 +1279,4 @@ 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()); - } } From e8082058880e662f583295f10cccf733bdc53378 Mon Sep 17 00:00:00 2001 From: "terence.yoo" Date: Thu, 12 Jan 2023 16:56:29 +0900 Subject: [PATCH 4/5] Close region after warmup --- .../apache/hadoop/hbase/regionserver/HRegion.java | 4 +++- .../apache/hadoop/hbase/regionserver/HStore.java | 5 ++++- .../hadoop/hbase/master/TestWarmupRegion.java | 14 +++++++++++++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 08987ea6b194..911f35fbd1c6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -7756,7 +7756,7 @@ public static HRegion openReadOnlyFileSystemHRegion(final Configuration conf, fi return r.openHRegion(null); } - public static void warmupHRegion(final RegionInfo info, final TableDescriptor htd, final WAL wal, + public static HRegion warmupHRegion(final RegionInfo info, final TableDescriptor htd, final WAL wal, final Configuration conf, final RegionServerServices rsServices, final CancelableProgressable reporter) throws IOException { @@ -7773,6 +7773,8 @@ public static void warmupHRegion(final RegionInfo info, final TableDescriptor ht } HRegion r = HRegion.newHRegion(tableDir, wal, fs, conf, info, htd, null); r.initializeWarmup(reporter); + r.close(); + return r; } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java index 5a2a74a61a46..5b52ab07a482 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java @@ -243,6 +243,8 @@ public Set get() { // SFT implementations. private final Supplier storeFileWriterCreationTrackerFactory; + private final boolean warmup; + /** * Constructor * @param family HColumnDescriptor for this column @@ -290,6 +292,7 @@ protected HStore(final HRegion region, final ColumnFamilyDescriptor family, this.compactionCheckMultiplier = DEFAULT_COMPACTCHECKER_INTERVAL_MULTIPLIER; } + this.warmup = warmup; this.storeEngine = createStoreEngine(this, this.conf, region.getCellComparator()); storeEngine.initialize(warmup); // if require writing to tmp dir first, then we just return null, which indicate that we do not @@ -740,7 +743,7 @@ private ImmutableCollection closeWithoutLock() throws IOException { public Void call() throws IOException { boolean evictOnClose = getCacheConfig() != null ? getCacheConfig().shouldEvictOnClose() : true; - f.closeStoreFile(evictOnClose); + f.closeStoreFile(!warmup && evictOnClose); return null; } }); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestWarmupRegion.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestWarmupRegion.java index 420c0a6cac60..95b3c6dd3ae6 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestWarmupRegion.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestWarmupRegion.java @@ -120,7 +120,7 @@ public boolean evaluate() throws IOException { */ @After public void tearDown() throws Exception { - // Nothing to do. + TEST_UTIL.deleteTable(TABLENAME); } protected void runwarmup() throws InterruptedException { @@ -163,4 +163,16 @@ public void testWarmup() throws Exception { serverid = (serverid + 1) % 2; } } + + @Test + public void testWarmupAndClose() throws IOException { + HRegionServer rs = TEST_UTIL.getMiniHBaseCluster().getRegionServer(0); + HRegion region = TEST_UTIL.getMiniHBaseCluster().getRegions(TABLENAME).get(0); + RegionInfo info = region.getRegionInfo(); + + TableDescriptor htd = table.getDescriptor(); + HRegion warmedUpRegion = + warmupHRegion(info, htd, rs.getWAL(info), rs.getConfiguration(), rs, null); + assertTrue(warmedUpRegion.isClosed()); + } } From 77e44f5300682d6708b65c203e35086060abfcb2 Mon Sep 17 00:00:00 2001 From: "terence.yoo" Date: Mon, 16 Jan 2023 19:01:35 +0900 Subject: [PATCH 5/5] fix a violation --- .../java/org/apache/hadoop/hbase/regionserver/HRegion.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 911f35fbd1c6..08b951a0eaf7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -7756,8 +7756,8 @@ public static HRegion openReadOnlyFileSystemHRegion(final Configuration conf, fi return r.openHRegion(null); } - public static HRegion warmupHRegion(final RegionInfo info, final TableDescriptor htd, final WAL wal, - final Configuration conf, final RegionServerServices rsServices, + public static HRegion warmupHRegion(final RegionInfo info, final TableDescriptor htd, + final WAL wal, final Configuration conf, final RegionServerServices rsServices, final CancelableProgressable reporter) throws IOException { Objects.requireNonNull(info, "RegionInfo cannot be null");