Copilot commented on code in PR #8569:
URL: https://github.com/apache/hbase/pull/8569#discussion_r3873607186
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -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;
Review Comment:
The cleanup `finally` only wraps the third-scan assertion block. If
`scanStarted.await(...)` fails (or any earlier assertion throws) after
`scanThread.start()`, the test can exit without releasing `allowScanToFinish`,
leaving `scanThread` blocked and potentially hanging the test suite. Wrap
everything from `scanThread.start()` onward in a broader `try/finally` that
always releases `allowScanToFinish` and joins/interrupts the thread.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -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);
Review Comment:
The cleanup `finally` only wraps the third-scan assertion block. If
`scanStarted.await(...)` fails (or any earlier assertion throws) after
`scanThread.start()`, the test can exit without releasing `allowScanToFinish`,
leaving `scanThread` blocked and potentially hanging the test suite. Wrap
everything from `scanThread.start()` onward in a broader `try/finally` that
always releases `allowScanToFinish` and joins/interrupts the thread.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -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();
Review Comment:
`allowScanToFinish.await()` has no timeout, so any failure path that misses
`countDown()` can block forever. Use a bounded wait (e.g., `await(timeout,
unit)`) and fail the test (or throw) with a clear message if the latch is not
released in time to avoid indefinite hangs.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -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);
+ }
+ );
Review Comment:
A 1-minute `assertTimeoutPreemptively` makes failures very slow and can also
leave background work in an interrupted state if the timeout triggers. Consider
using a much smaller timeout (seconds-level) and/or `assertTimeout`
(non-preemptive) if interruption is not required, since `spy.scan()` should
return immediately with `-1` when the lock is held.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
