diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java index 14cf61ef970b..a9fb510ca397 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java @@ -164,14 +164,14 @@ private static boolean isRIT(AssignmentManager am) { */ public int scan() throws IOException { int gcs = 0; - try { - if (!alreadyRunning.compareAndSet(false, true)) { - if (LOG.isDebugEnabled()) { - LOG.debug("CatalogJanitor already running"); - } - // -1 indicates previous scan is in progress - return -1; + if (!alreadyRunning.compareAndSet(false, true)) { + if (LOG.isDebugEnabled()) { + LOG.debug("CatalogJanitor already running"); } + // -1 indicates previous scan is in progress + return -1; + } + try { this.lastReport = scanForReport(); if (!this.lastReport.isEmpty()) { LOG.warn(this.lastReport.toString()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java index 259ff1636bf9..8ef72bbb6157 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java @@ -20,6 +20,7 @@ import static org.apache.hadoop.hbase.util.HFileArchiveTestingUtil.assertArchiveEqualToOriginal; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -29,12 +30,15 @@ import static org.mockito.Mockito.when; import java.io.IOException; +import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.SortedMap; import java.util.TreeMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FileStatus; @@ -690,6 +694,63 @@ public void testAlreadyRunningStatus() throws Exception { assertTrue(gcValues.contains(-1), "One janitor.scan() call should have returned -1"); } + @Test + public void testAlreadyRunningStatusDoesNotClearLock() throws Exception { + CatalogJanitor spy = spy(this.janitor); + + CountDownLatch scanStarted = new CountDownLatch(1); + CountDownLatch allowScanToFinish = new CountDownLatch(1); + + doAnswer(invocation -> { + scanStarted.countDown(); + allowScanToFinish.await(); + return new CatalogJanitorReport(); + }).when(spy).scanForReport(); + + Thread scanThread = new Thread(() -> { + try { + spy.scan(); + } catch (IOException e) { + throw new RuntimeException(e); + } + }); + + // First scan acquires the lock and remains running. + scanThread.start(); + assertTrue(scanStarted.await(5, TimeUnit.SECONDS)); + LOG.info("First catalog janitor scan started and waiting to finish."); + + // Second scan detects that another scan is running. + assertEquals(-1, spy.scan()); + LOG.info("Second catalog janitor scan attempt returned -1."); + + // The second scan must not clear the lock. + // Therefore, the third scan must also report that a scan is running. + try { + assertTimeoutPreemptively( + Duration.ofMinutes(1), + () -> { + int result = spy.scan(); + LOG.info("Third catalog janitor scan attempt returned {}.", result); + assertEquals(-1, result); + } + ); + } catch (AssertionError e) { + LOG.error( + "Third catalog janitor scan did not return -1 within 60 seconds; " + + "the scan may be running instead of returning -1.", + e + ); + throw e; + } finally { + // Let the first scan finish. + LOG.info("Releasing first catalog janitor scan and waiting for it to complete."); + allowScanToFinish.countDown(); + scanThread.join(5000); + assertFalse(scanThread.isAlive()); + } + } + private FileStatus[] addMockStoreFiles(int count, MasterServices services, Path storedir) throws IOException { // get the existing store files