guluo2016 commented on code in PR #8569:
URL: https://github.com/apache/hbase/pull/8569#discussion_r3881961971
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -690,6 +694,67 @@ 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();
+ assertTrue(
+ allowScanToFinish.await(15, TimeUnit.SECONDS),
+ "Timed out waiting for the test to release the first catalog janitor
scan."
+ );
+ return new CatalogJanitorReport();
+ }).when(spy).scanForReport();
+
+ Thread scanThread = new Thread(() -> {
+ try {
+ spy.scan();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ });
+
+ scanThread.start();
+ try {
+ // First scan acquires the lock and remains running.
+ 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.
+
+ assertTimeoutPreemptively(
Review Comment:
I think the assertTimeoutPreemptively could be removed, since spy.scan()
should return -1 immediately without needing a background thread.
In addition, the assertTimeoutPreemptively has undesirable side effects in
some scenarios，details
see：https://docs.junit.org/6.0.0/writing-tests/assertions.html?utm_source=chatgpt.com#:~:text=Preemptive%20Timeouts%20with,lang.ThreadLocal%20storage.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -690,6 +694,67 @@ 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();
+ assertTrue(
+ allowScanToFinish.await(15, TimeUnit.SECONDS),
+ "Timed out waiting for the test to release the first catalog janitor
scan."
+ );
+ return new CatalogJanitorReport();
+ }).when(spy).scanForReport();
+
+ Thread scanThread = new Thread(() -> {
+ try {
+ spy.scan();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ });
+
+ scanThread.start();
+ try {
+ // First scan acquires the lock and remains running.
+ 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.
+
+ assertTimeoutPreemptively(
+ Duration.ofSeconds(5),
+ () -> {
+ int result = spy.scan();
+ LOG.info("Third catalog janitor scan attempt returned {}.", result);
+ assertEquals(-1, result);
+ }
+ );
+ } catch (AssertionError e) {
+ LOG.error(
+ "Catalog janitor scan concurrency test failed; "
+ + "the alreadyRunning lock mechanism may not be behaving as
expected.",
+ e
+ );
+ throw e;
Review Comment:
I think this catch block also is unnecessary, since it doesn't add any
assertion or change the test result.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java:
##########
@@ -690,6 +694,67 @@ 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();
+ assertTrue(
+ allowScanToFinish.await(15, TimeUnit.SECONDS),
+ "Timed out waiting for the test to release the first catalog janitor
scan."
+ );
+ return new CatalogJanitorReport();
+ }).when(spy).scanForReport();
+
+ Thread scanThread = new Thread(() -> {
+ try {
+ spy.scan();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ });
+
+ scanThread.start();
+ try {
+ // First scan acquires the lock and remains running.
+ 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.
+
+ assertTimeoutPreemptively(
+ Duration.ofSeconds(5),
+ () -> {
+ int result = spy.scan();
+ LOG.info("Third catalog janitor scan attempt returned {}.", result);
+ assertEquals(-1, result);
+ }
+ );
+ } catch (AssertionError e) {
+ LOG.error(
+ "Catalog janitor scan concurrency test failed; "
+ + "the alreadyRunning lock mechanism may not be behaving as
expected.",
+ 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());
Review Comment:
The assertion also is unnecessary
Maybe we can simply log whether the scanThread is alive here.
--
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]
