sercanCyberVision commented on code in PR #8569:
URL: https://github.com/apache/hbase/pull/8569#discussion_r3882144175
##########
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:
You are right. We already have `allowScanToFinish.await(15,
TimeUnit.SECONDS),`, which provides a timeout if the third scan incorrectly
acquires the lock and starts running. I have removed `assertTimeoutPreemptively`
##########
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:
It was only there for logging purposes for the third scan attempt. Since
`assertTimeoutPreemptively` has been removed, it is no longer needed, so I
removed it as well.
##########
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:
Done.
--
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]
