HDDS-13320. BackgroundService should wait for the completion of run before triggering next run #8677

swamirishi · 2025-06-22T23:48:15Z

What changes were proposed in this pull request?

Currently even though the java doc of the base class BackgroundService says it would wait for the entire run to finish before triggering the next run, it doesn't wait as it just submits the tasks to the queue and doesn't track if those tasks have finished.
Added a wait at the end of CompletableFuture to fix it.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13320

How was this patch tested?

No unit tests added unit test for the same.

…efore triggering next run Change-Id: I2569ad4a0ab27e900c4f6d3cae98b0e4735503ca

Change-Id: I5f4e826ca73cc43d5e81b4df7289382bf31cf510

sadanand48 · 2025-06-23T06:59:57Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/util/TestBackgroundService.java

+    }, 100, 3000);
+    Thread.sleep(3000);
+    assertEquals(values, IntStream.range(0, 10).boxed().map(map::get).collect(Collectors.toList()));
+    lockList.forEach(Lock::unlock);


minor nit :
check that the even indices are also processed after unlock and maybe check for size of values (5)before and after unlock (10)

adoroszlai

Thanks @swamirishi for the patch.

adoroszlai · 2025-06-23T06:11:08Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java

+      } catch (Throwable e) {
+        LOG.error("Background service execution failed", e);
+      } finally {


Re-throw Error.

adoroszlai · 2025-06-23T06:11:43Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java

+      } finally {
+        long endTime = System.nanoTime();
+        if (endTime - serviceStartTime > serviceTimeoutInNanos) {
+          LOG.warn("{} Background service execution failed which took {}ns > {}ns(timeout)",


Are you sure we get here only if execution failed?

adoroszlai · 2025-06-23T06:14:10Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/util/TestBackgroundService.java

+      }
+      return true;
+    }, 100, 3000);
+    Thread.sleep(3000);


Please don't use fixed sleep.

adoroszlai · 2025-06-23T06:14:22Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/util/TestBackgroundService.java

+    lockList.forEach(Lock::unlock);
+    backgroundService.shutdown();


Release/shutdown in finally.

adoroszlai · 2025-06-23T06:49:28Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/util/TestBackgroundService.java

+        if (i % 2 == 1 && map.get(i) != 1) {
+          return false;
+        }
+        if (i % 2 == 0 && map.get(i) != 0) {
+          return false;
+        }


i % 2 and map.get(i) can be compared directly.

adoroszlai · 2025-06-23T07:08:44Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/util/TestBackgroundService.java

+    Map<Integer, Integer> map = new HashMap<>();
+    Map<Integer, Lock> locks = new HashMap<>();
+    for (int i = 0; i < 10; i++) {
+      int index = i;
+      locks.put(index, new ReentrantLock());
+      map.put(index, 0);
+      queue.add(() -> {
+        locks.get(index).lock();
+        try {
+          map.compute(index, (k, v) -> v == null ? 1 : (v + 1));
+        } finally {
+          locks.get(index).unlock();
+        }
+        return new BackgroundTaskResult.EmptyTaskResult();
+      });
+    }


To simplify the test case, please extract an inner class for the task, which should keep track of its invocations, and support locking.

SaketaChalamchala · 2025-07-14T21:23:42Z

Thanks for the review @sadanand48 and @adoroszlai. Could you review the updated patch?

SaketaChalamchala · 2025-07-17T17:43:56Z

@jojochuang and @smengcl could you also review this patch?

adoroszlai · 2025-07-22T05:25:23Z

Please do take a look at CI results in fork.

M M IS: Inconsistent synchronization of org.apache.hadoop.hdds.utils.BackgroundService.exec; locked 83% of time  Unsynchronized access at BackgroundService.java:[line 189]
M D IM: Check for oddness that won't work for negative numbers in org.apache.hadoop.util.TestBackgroundService.lambda$null$3(int)  At TestBackgroundService.java:[line 131]

https://github.com/swamirishi/ozone/actions/runs/16352154299/job/46201433990

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java
 23: Unused import - java.util.concurrent.CompletionException.

https://github.com/swamirishi/ozone/actions/runs/16352154299/job/46201434007#step:16:17

jojochuang

LGTM

… finish before launching next set and exiting.

SaketaChalamchala · 2025-07-25T17:09:06Z

Updated the patch. Instead of the runner waiting for all the tasks to complete at the end,

Runner waits at the beginning for the previous set of tasks to complete
Runner Submits the next set of tasks to the thread pool and exits.
This way, even with a single thread the worker tasks get to complete because runner just submits the task and exits and the next scheduled run will not start until all the previous tasks are complete.
cc @swamirishi
@jojochuang , @adoroszlai @smengcl @sadanand48 could you please review the update?

jojochuang · 2025-07-25T22:18:53Z

findbugs: M M IS: Inconsistent synchronization of org.apache.hadoop.hdds.utils.BackgroundService.exec; locked 75% of time Unsynchronized access at BackgroundService.java:[line 92]

@SaketaChalamchala please run ./hadoop-ozone/dev-support/checks/findbugs.sh locally to confirm the next fix. Thanks!

Change-Id: I1dd2167205b3ba95a871882eb29b312387aa70d3

…ache#8677.

swamirishi

LGTM excepting for a few nitpicky comments.

swamirishi · 2025-07-28T15:12:36Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java


  public abstract BackgroundTaskQueue getTasks();

+  protected void execTaskCompletion() { }


I don't think this is required

I added this so that it might be useful to perform post task completion steps like updating last run metrics or incrementing run count or processing the result of the background tasks if necessary

swamirishi · 2025-07-28T15:13:02Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java

+      } catch (RuntimeException e) {
+        LOG.error("Background service execution failed.", e);
+      } finally {
+        execTaskCompletion();


We can simply addAndIncreament while submitting

jojochuang · 2025-07-30T17:49:28Z

Looks good to me. I'll merge it to unblock #8655

jojochuang · 2025-07-30T17:50:07Z

Thanks @SaketaChalamchala @swamirishi @sadanand48 @adoroszlai

adoroszlai · 2025-07-30T19:04:26Z

Author:     Swaminathan Balachandran <swamirishi.sb@gmail.com>
AuthorDate: Wed Jul 30 13:49:54 2025 -0400
Commit:     GitHub <noreply@github.com>
CommitDate: Wed Jul 30 10:49:54 2025 -0700

    HDDS-13320. BackgroundService should wait for the completion of run before triggering next run (#8677)
    
    Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>

When merging, please make sure to credit contributors. @SaketaChalamchala had 7 of 10 commits in this PR.

…ache#8677.

…efore triggering next run (apache#8677) Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>

adoroszlai · 2025-07-31T07:00:43Z

...ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java

-    List<? extends DatanodeDetails> dnList = cluster().getStorageContainerManager()
-            .getScmNodeManager()
-            .getAllNodes();
+    NodeManager nodeManager = cluster().getStorageContainerManager().getScmNodeManager();
+    List<? extends DatanodeDetails> dnList = nodeManager.getAllNodes();

    for (DatanodeDetails dn : dnList) {
-      final int expected = cluster().getStorageContainerManager().getScmNodeManager().getContainers(dn).size();
-
      List<HddsProtos.DatanodeUsageInfoProto> usageInfoList =
              storageClient.getDatanodeUsageInfo(
                      dn.getIpAddress(), dn.getUuidString());

      assertEquals(1, usageInfoList.size());
-      assertEquals(expected, usageInfoList.get(0).getContainerCount());
+      assertEquals(nodeManager.getContainers(dn).size(), usageInfoList.get(0).getContainerCount());


This change in TestContainerOperations seems completely unrelated to the rest of the PR.

…efore triggering next run (apache#8677) Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>

…ion of run before triggering next run (apache#8677) Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org> (cherry picked from commit ce5676a) Conflicts: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java hadoop-hdds/common/src/main/java/org/apache/hadoop/util/TestBackgroundService.java hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java Change-Id: Ie41fb7a56755364a31aefe6d8936a5849bf03b42

swamirishi added 2 commits June 22, 2025 19:06

HDDS-13320. BackgroundService should wait for the completion of run b…

9a1f5fe

…efore triggering next run Change-Id: I2569ad4a0ab27e900c4f6d3cae98b0e4735503ca

HDDS-13320. Add tests

ecbf33c

Change-Id: I5f4e826ca73cc43d5e81b4df7289382bf31cf510

swamirishi requested review from ashishkumar50, sadanand48 and sumitagrawl June 22, 2025 23:48

sadanand48 reviewed Jun 23, 2025

View reviewed changes

adoroszlai reviewed Jun 23, 2025

View reviewed changes

SaketaChalamchala mentioned this pull request Jul 2, 2025

HDDS-12887. Added Deletion Progress metrics to OM Web UI. #8655

Merged

HDDS-13320. Seperated runner and worker threadpool to avoid deadlocks.

b1d7108

HDDS-13320. Using future.join().

b3fd189

HDDS-13320. Fixed checkstyle and findbugs issues.

57f39f9

jojochuang approved these changes Jul 22, 2025

View reviewed changes

jojochuang marked this pull request as ready for review July 24, 2025 15:17

HDDS-13320. Resolved deadlock by waiting for previous set of tasks to…

4062654

… finish before launching next set and exiting.

saketa and others added 3 commits July 25, 2025 18:48

HDDS-13320. Made setPoolSize synchronized.

7f746af

Fix findbugs

92cae54

Change-Id: I1dd2167205b3ba95a871882eb29b312387aa70d3

HDDS-13320. Fix more findbugs.

68e11d6

SaketaChalamchala pushed a commit to SaketaChalamchala/ozone that referenced this pull request Jul 26, 2025

HDDS-12887. Updated UI and patch to highlight 24h summary based on ap…

9ef29dd

…ache#8677.

swamirishi commented Jul 28, 2025

View reviewed changes

smengcl added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jul 28, 2025

HDDS-13320. Fix testDatanodeUsageInfoContainerCount() flaky test

1139344

jojochuang merged commit ce5676a into apache:master Jul 30, 2025
42 checks passed

SaketaChalamchala pushed a commit to SaketaChalamchala/ozone that referenced this pull request Jul 30, 2025

HDDS-12887. Updated UI and patch to highlight 24h summary based on ap…

3f26660

…ache#8677.

jojochuang added a commit to jojochuang/ozone that referenced this pull request Jul 31, 2025

HDDS-13320. BackgroundService should wait for the completion of run b…

cc7284a

…efore triggering next run (apache#8677) Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>

adoroszlai reviewed Jul 31, 2025

View reviewed changes

adoroszlai mentioned this pull request Jul 31, 2025

HDDS-13310. Intermittent failure in TestContainerOperations #8770

Closed

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Jul 31, 2025

HDDS-13320. BackgroundService should wait for the completion of run b…

afb398c

…efore triggering next run (apache#8677) Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>

		lockList.forEach(Lock::unlock);
		backgroundService.shutdown();


		public abstract BackgroundTaskQueue getTasks();

		protected void execTaskCompletion() { }

HDDS-13320. BackgroundService should wait for the completion of run before triggering next run #8677

HDDS-13320. BackgroundService should wait for the completion of run before triggering next run #8677

Uh oh!

Conversation

swamirishi commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaketaChalamchala commented Jul 14, 2025

Uh oh!

SaketaChalamchala commented Jul 17, 2025

Uh oh!

adoroszlai commented Jul 22, 2025

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

SaketaChalamchala commented Jul 25, 2025

Uh oh!

jojochuang commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jojochuang commented Jul 30, 2025

Uh oh!

Uh oh!

jojochuang commented Jul 30, 2025

Uh oh!

adoroszlai commented Jul 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

swamirishi commented Jun 22, 2025 •

edited

Loading

jojochuang commented Jul 25, 2025 •

edited

Loading