HDDS-14020. Use ForkJoinPool instead of a ScheduledThreadPoolExecutor in BackgroundService #9390
Conversation
swamirishi
changed the title
swamirishi
force-pushed
the
HDDS-14020_FJP_Change
branch
2 times, most recently
from
2be9d46 to
ee29c5f
Compare
|
@SaketaChalamchala do you wanna review this? |
swamirishi
force-pushed
the
HDDS-14020_FJP_Change
branch
from
ee29c5f to
4d431fd
Compare
adoroszlai
changed the title
swamirishi
force-pushed
the
HDDS-14020_FJP_Change
branch
4 times, most recently
from
9212b98 to
579eac8
Compare
…rvice Change-Id: I4c1a051e8574d32375cbebeb10546563f4a4f817
swamirishi
force-pushed
the
HDDS-14020_FJP_Change
branch
from
579eac8 to
14d25f1
Compare
sumitagrawl
left a comment
sumitagrawl
left a comment
There was a problem hiding this comment.
@swamirishi given few comments, may need change in such a way to avoid changes all other places for change in execution way.
| if (tasks.isEmpty()) { | ||
| // No task found, or some problems to init tasks | ||
| // return and retry in next interval. | ||
| continue; |
There was a problem hiding this comment.
this can cause continuous loop as directly jump to while condition without waitForNextInterval()
| * A task thread to run by {@link BackgroundService}. | ||
| */ | ||
| public interface BackgroundTask extends Callable<BackgroundTaskResult> { | ||
| public abstract class BackgroundTask extends RecursiveTask<BackgroundTask.BackgroundTaskForkResult> |
There was a problem hiding this comment.
need not change BackgroundTask, but encapsulate in inside ForkJoinTask as transient and can be scheduled. This avoid changing all other place due to change in framework. Externally, its just Runnable task.
There was a problem hiding this comment.
The external task has to be a recursive task otherwise forkjoin pool cannot work
There was a problem hiding this comment.
We can do like below, please check... BackgroundTaskForkJoin will not be exposed.
public class BackgroundTaskForkJoin extends RecursiveTask<BackgroundTaskForkJoin.BackgroundTaskForkResult> {
private static final long serialVersionUID = 1L;
private final transient BackgroundTask backgroundTask;
public BackgroundTaskForkJoin(BackgroundTask backgroundTask) {
this.backgroundTask = backgroundTask;
}
public static class BackgroundTaskForkResult {
...
}
}
There was a problem hiding this comment.
Since this one is closed. I've raised #9686 that addressed the comment above.
| } | ||
| } | ||
| } | ||
| future.complete(null); |
There was a problem hiding this comment.
unable to get use of future here
| while (!tasksInFlight.isEmpty()) { | ||
| BackgroundTask taskInFlight = tasksInFlight.poll(); | ||
| // Join the tasks forked before and wait for the result one by one. | ||
| BackgroundTask.BackgroundTaskForkResult result = taskInFlight.join(); |
There was a problem hiding this comment.
Do the task is cancellable? if crosses certain time
There was a problem hiding this comment.
We can implement something if we want to. But this is not supported even today.
Change-Id: Ie2118356f902443a93fe666d890ad4d59e9dd467
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it. |
|
@sumitagrawl Would you like to take another look? |
…omment: apache#9390 (comment) Refactor BackgroundTask to use wrapper pattern for ForkJoinPool integration This commit introduces BackgroundTaskForkJoin as a wrapper class to integrate BackgroundTask with ForkJoinPool, avoiding the need to change all service implementations from 'implements' to 'extends'. Key changes: - Reverted BackgroundTask from abstract class back to interface - Created BackgroundTaskForkJoin wrapper extending RecursiveTask - Updated BackgroundService to wrap tasks before forking - Reverted all service task classes to 'implements BackgroundTask'
4 participants
What changes were proposed in this pull request?
Currently both background task and shutdown acquire a synchronized lock which can lead to a deadlock on shutdown which means the threadPool shutdown takes time unnecessarily. The synchronized block on task was taken to ensure there are no parallel tasks that run together.
This patch removes the unnecessary synchronized calls and converts the ThreadPoolExecutor to a fork join pool. Instead of hacking around with the wait logic present at the start of a periodic task to avoid deadlock in case of a single threaded background service we can use a forkJoin pool to fork on to background and task and join all the forked task to implement the wait.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14020
How was this patch tested?
Existing tests should be sufficient