You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "njnu-seafish (via GitHub)" <gi...@apache.org> on 2025/12/15 12:01:53 UTC
[PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
njnu-seafish opened a new pull request, #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796
<!--Thanks very much for contributing to Apache DolphinScheduler, we are happy that you want to help us improve DolphinScheduler! -->
## Purpose of the pull request
close #17795
## Brief change log
Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available
## Verify this pull request
Successfully tested and verified in a real-world environment.
## Pull Request Notice
[Pull Request Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
If your pull request contains incompatible change, you should also add it to `docs/docs/en/guide/upgrade/incompatible.md`
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3822181341
@SbloodyS Whenever you have time, I’d be grateful for your review. Thanks so much!
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2684645579
##########
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcherTest.java:
##########
@@ -46,7 +47,8 @@ class WorkerGroupDispatcherTest {
@BeforeEach
void setUp() {
taskExecutorClient = mock(ITaskExecutorClient.class);
- dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient);
+ final MasterConfig masterConfig = new MasterConfig();
+ dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient, masterConfig.getDispatchTimeout());
Review Comment:
> You should add a validated test case.
The following integration test produces an unexpected result because the LogicFakeTask type runs on the Master and does not involve WorkerGroup dispatching.
I track the fix in a separate issue: https://github.com/apache/dolphinscheduler/issues/17872
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2715585636
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -97,6 +99,18 @@ public void validate(Object target, Errors errors) {
if (masterConfig.getWorkerGroupRefreshInterval().getSeconds() < 10) {
errors.rejectValue("worker-group-refresh-interval", null, "should >= 10s");
}
+
+ TaskDispatchPolicy dispatchPolicy = masterConfig.getTaskDispatchPolicy();
+ if (dispatchPolicy != null && dispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ if (dispatchPolicy.getMaxTaskDispatchDuration() == null) {
+ errors.rejectValue("dispatch-timeout-checker.max-task-dispatch-duration", null,
+ "must be specified when dispatch timeout checker is enabled");
+ } else if (dispatchPolicy.getMaxTaskDispatchDuration().toMillis() <= 0) {
+ errors.rejectValue("dispatch-timeout-checker.max-task-dispatch-duration", null,
+ "must be a positive duration (e.g., '2m', '5m', '30m')");
+ }
+ }
Review Comment:
Can dispatchPolicy be null?
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +93,69 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (Exception ex) {
+ if (taskDispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ // If a dispatch timeout occurs, the task will not be put back into the queue.
+ long timeoutMs = this.taskDispatchPolicy.getMaxTaskDispatchDuration().toMillis();
Review Comment:
https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2623030826
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +93,69 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (Exception ex) {
+ if (taskDispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ // If a dispatch timeout occurs, the task will not be put back into the queue.
+ long timeoutMs = this.taskDispatchPolicy.getMaxTaskDispatchDuration().toMillis();
+ long elapsed = System.currentTimeMillis() - taskExecutionContext.getFirstDispatchTime();
+ if (elapsed > timeoutMs) {
+ handleDispatchFailure(taskExecutionRunnable, ex, elapsed, timeoutMs);
+ return;
+ }
+ }
+
// If dispatch failed, will put the task back to the queue
// The task will be dispatched after waiting time.
// the waiting time will increase multiple of times, but will not exceed 60 seconds
- long waitingTimeMills = Math.min(
+ long waitingTimeMillis = Math.min(
taskExecutionRunnable.getTaskExecutionContext().increaseDispatchFailTimes() * 1_000L, 60_000L);
- dispatchTask(taskExecutionRunnable, waitingTimeMills);
- log.error("Dispatch Task: {} failed will retry after: {}/ms", taskExecutionRunnable.getId(),
- waitingTimeMills, e);
+ dispatchTask(taskExecutionRunnable, waitingTimeMillis);
+ log.warn("Dispatch Task: {} failed will retry after: {}/ms", taskInstanceId,
+ waitingTimeMillis, ex);
+ }
+ }
+
+ /**
+ * Marks the specified task as fatally failed due to an unrecoverable dispatch error,such as timeout
+ * Once this method is called, the task is considered permanently failed and will not be retried.
+ */
+ private void handleDispatchFailure(ITaskExecutionRunnable taskExecutionRunnable, Exception ex,
+ long elapsed, long timeoutMs) {
+ final String taskName = taskExecutionRunnable.getName();
+
+ log.warn("Dispatch fail, taskName: {}, timed out after {} ms (limit: {} ms))", taskName, elapsed, timeoutMs);
+
+ if (ExceptionUtils.isWorkerGroupNotFoundException(ex)) {
+ log.error("Dispatch fail, taskName: {}, Worker group not found.", taskName, ex);
+ final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
+ .taskExecutionRunnable(taskExecutionRunnable)
+ .endTime(new Date())
+ .build();
+ taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
+ } else if (ExceptionUtils.isNoAvailableWorkerException(ex)) {
+ log.error("Dispatch fail, taskName: {}, No available worker.", taskName, ex);
+ final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
+ .taskExecutionRunnable(taskExecutionRunnable)
+ .endTime(new Date())
+ .build();
+ taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
+ } else {
+ log.error("Dispatch fail, taskName: {}, Unexpected dispatch error.", taskName, ex);
+ final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
+ .taskExecutionRunnable(taskExecutionRunnable)
+ .endTime(new Date())
+ .build();
+ taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
Review Comment:
```suggestion
log.error("Task: {} dispatch timeout.", taskName, ex);
final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
.taskExecutionRunnable(taskExecutionRunnable)
.endTime(new Date())
.build();
taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
```
##########
dolphinscheduler-master/src/main/resources/application.yaml:
##########
@@ -111,6 +111,11 @@ master:
# Master max concurrent workflow instances, when the master's workflow instance count exceeds this value, master server will be marked as busy.
max-concurrent-workflow-instances: 2147483647
worker-group-refresh-interval: 5m
+ # Task dispatch timeout check (currently disabled).
+ # When enabled, tasks not dispatched within this duration are marked as failed.
+ task-dispatch-policy:
+ dispatch-timeout-failed-enabled: false
+ max-task-dispatch-duration: 5m
Review Comment:
```suggestion
max-task-dispatch-duration: 1h
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/TaskDispatchPolicy.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch policy.
+ * When enabled, tasks that remain in the dispatch queue longer than
+ * {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class TaskDispatchPolicy {
+
+ /**
+ * Indicates whether the dispatch timeout checking mechanism is enabled.
+ */
+ private boolean dispatchTimeoutFailedEnabled = false;
Review Comment:
```suggestion
private boolean dispatchTimeoutEnabled = false;
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +93,69 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (Exception ex) {
+ if (taskDispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ // If a dispatch timeout occurs, the task will not be put back into the queue.
+ long timeoutMs = this.taskDispatchPolicy.getMaxTaskDispatchDuration().toMillis();
+ long elapsed = System.currentTimeMillis() - taskExecutionContext.getFirstDispatchTime();
+ if (elapsed > timeoutMs) {
+ handleDispatchFailure(taskExecutionRunnable, ex, elapsed, timeoutMs);
+ return;
+ }
+ }
+
// If dispatch failed, will put the task back to the queue
// The task will be dispatched after waiting time.
// the waiting time will increase multiple of times, but will not exceed 60 seconds
- long waitingTimeMills = Math.min(
+ long waitingTimeMillis = Math.min(
taskExecutionRunnable.getTaskExecutionContext().increaseDispatchFailTimes() * 1_000L, 60_000L);
- dispatchTask(taskExecutionRunnable, waitingTimeMills);
- log.error("Dispatch Task: {} failed will retry after: {}/ms", taskExecutionRunnable.getId(),
- waitingTimeMills, e);
+ dispatchTask(taskExecutionRunnable, waitingTimeMillis);
+ log.warn("Dispatch Task: {} failed will retry after: {}/ms", taskInstanceId,
+ waitingTimeMillis, ex);
+ }
+ }
+
+ /**
+ * Marks the specified task as fatally failed due to an unrecoverable dispatch error,such as timeout
+ * Once this method is called, the task is considered permanently failed and will not be retried.
+ */
+ private void handleDispatchFailure(ITaskExecutionRunnable taskExecutionRunnable, Exception ex,
Review Comment:
```suggestion
private void onDispatchTimeout(ITaskExecutionRunnable taskExecutionRunnable, Exception ex,
```
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2719767178
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/TaskDispatchPolicy.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch policy.
+ * When enabled, tasks that remain in the dispatch queue longer than
+ * {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class TaskDispatchPolicy {
+
+ /**
+ * Indicates whether the dispatch timeout checking mechanism is enabled.
+ */
+ private boolean dispatchTimeoutFailedEnabled = false;
Review Comment:
> dispatchTimeoutEnabled
better
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2719763888
##########
dolphinscheduler-master/src/main/resources/application.yaml:
##########
@@ -111,6 +111,11 @@ master:
# Master max concurrent workflow instances, when the master's workflow instance count exceeds this value, master server will be marked as busy.
max-concurrent-workflow-instances: 2147483647
worker-group-refresh-interval: 5m
+ # Task dispatch timeout check (currently disabled).
+ # When enabled, tasks not dispatched within this duration are marked as failed.
+ task-dispatch-policy:
+ dispatch-timeout-failed-enabled: false
+ max-task-dispatch-duration: 5m
Review Comment:
> 1h
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2623065956
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/exception/dispatch/WorkerGroupNotFoundException.java:
##########
@@ -19,7 +19,7 @@
public class WorkerGroupNotFoundException extends TaskDispatchException {
- public WorkerGroupNotFoundException(String workerGroup) {
- super("Cannot find worker group: " + workerGroup);
+ public WorkerGroupNotFoundException(String message) {
Review Comment:
Why do this change?
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/TaskExecutionContext.java:
##########
@@ -126,6 +126,11 @@ public class TaskExecutionContext implements Serializable {
private boolean failover;
+ /**
+ * Timestamp (ms) when the task was first enqueued for dispatch.
+ */
+ private final long firstDispatchEnqueueTimeMs = System.currentTimeMillis();
Review Comment:
```suggestion
private final long firstDispatchTime = System.currentTimeMillis();
```
This is more like the creation time of the `taskExecutionContext`.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
+ final long timeoutMs = this.dispatchTimeout.toMillis();
Review Comment:
It's better to store the mills rather than store duration, then you can avoid execute `dispatchTimeout.toMillis()` here.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -74,6 +74,12 @@ public class MasterConfig implements Validator {
*/
private String masterRegistryPath;
+ /**
+ * Maximum time allowed for a task to be successfully dispatched.
+ * Default: 5 minutes.
+ */
+ private Duration dispatchTimeout = Duration.ofMinutes(5);
Review Comment:
```suggestion
private Duration maxTaskDispatchDuration;
```
`dispatchTimeout` might confuse with single RPC timeout when dispatch task, and the default value should be null or a large duration, should compatibility with the previous behavior.
And you need to add this in document.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/exception/dispatch/WorkerNotFoundException.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.exception.dispatch;
+
+public class WorkerNotFoundException extends TaskDispatchException {
+
+ public WorkerNotFoundException(String message) {
Review Comment:
```suggestion
public NoAvailableWorkerException(String workerGroup) {
super("Cannot find available worker under worker group: " + workerGroup);
}
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
Review Comment:
```suggestion
final int taskExecutionRunnableId = taskExecutionRunnable.getId();
```
Avoid confuse with task definition id.
##########
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcherTest.java:
##########
@@ -46,7 +47,8 @@ class WorkerGroupDispatcherTest {
@BeforeEach
void setUp() {
taskExecutorClient = mock(ITaskExecutorClient.class);
- dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient);
+ final MasterConfig masterConfig = new MasterConfig();
+ dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient, masterConfig.getDispatchTimeout());
Review Comment:
You should add a validated test case.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
+ final long timeoutMs = this.dispatchTimeout.toMillis();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (TaskDispatchException ex) {
+ // Checks whether the given task has exceeded its allowed dispatch timeout.
+ long elapsed = System.currentTimeMillis() - taskExecutionContext.getFirstDispatchEnqueueTimeMs();
+ if (elapsed > timeoutMs) {
+ handleDispatchFailure(taskExecutionRunnable, ex, elapsed, timeoutMs);
+ return;
+ }
+
// If dispatch failed, will put the task back to the queue
// The task will be dispatched after waiting time.
// the waiting time will increase multiple of times, but will not exceed 60 seconds
- long waitingTimeMills = Math.min(
+ long waitingTimeMillis = Math.min(
taskExecutionRunnable.getTaskExecutionContext().increaseDispatchFailTimes() * 1_000L, 60_000L);
- dispatchTask(taskExecutionRunnable, waitingTimeMills);
- log.error("Dispatch Task: {} failed will retry after: {}/ms", taskExecutionRunnable.getId(),
- waitingTimeMills, e);
+ dispatchTask(taskExecutionRunnable, waitingTimeMillis);
+ log.warn("Dispatch Task: {} failed will retry after: {}/ms", taskId,
+ waitingTimeMillis, ex);
+ }
+ }
+
+ /**
+ * Marks the specified task as fatally failed due to an unrecoverable dispatch error,such as timeout or persistent client failure.
+ * Once this method is called, the task is considered permanently failed and will not be retried.
+ *
+ * @param taskExecutionRunnable the task to mark as fatally failed; must not be null
+ * @param exception the dispatch exception that triggered this failure handling; must not be null
+ * @param elapsed the time (in milliseconds) already spent attempting to dispatch the task
+ * @param timeoutMs the configured dispatch timeout threshold (in milliseconds)
+ */
+ private void handleDispatchFailure(ITaskExecutionRunnable taskExecutionRunnable, TaskDispatchException exception,
Review Comment:
Please don't print the `taskId` and `workflowId` here, all ids should already be added by MDC. We should only need to print the exception here, the `exception` already contains failure message.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3721666401
@SbloodyS When you have time, could you please help trigger the CI pipeline? I'm unable to run the integration tests locally. Thank you so much!
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3753779780
@ruanwenjun @SbloodyS Whenever you have time, I’d be grateful for your review. Thanks so much!
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3782418669
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3833700904
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3833691678
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3782413250
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2715661397
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -97,6 +99,18 @@ public void validate(Object target, Errors errors) {
if (masterConfig.getWorkerGroupRefreshInterval().getSeconds() < 10) {
errors.rejectValue("worker-group-refresh-interval", null, "should >= 10s");
}
+
+ TaskDispatchPolicy dispatchPolicy = masterConfig.getTaskDispatchPolicy();
+ if (dispatchPolicy != null && dispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ if (dispatchPolicy.getMaxTaskDispatchDuration() == null) {
+ errors.rejectValue("dispatch-timeout-checker.max-task-dispatch-duration", null,
+ "must be specified when dispatch timeout checker is enabled");
+ } else if (dispatchPolicy.getMaxTaskDispatchDuration().toMillis() <= 0) {
+ errors.rejectValue("dispatch-timeout-checker.max-task-dispatch-duration", null,
+ "must be a positive duration (e.g., '2m', '5m', '30m')");
+ }
+ }
Review Comment:
> Can dispatchPolicy be null?
It indeed cannot be null.
private TaskDispatchPolicy taskDispatchPolicy = new TaskDispatchPolicy();
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2638551184
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterDispatchTimeoutCheckerConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch timeout checker.
+ * If enabled, tasks that remain in the dispatch queue longer than {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class MasterDispatchTimeoutCheckerConfig {
+
+ /**
+ * Whether to enable the dispatch timeout checking mechanism.
+ */
+ private boolean enabled = false;
Review Comment:
>
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2719722703
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +93,69 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (Exception ex) {
+ if (taskDispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ // If a dispatch timeout occurs, the task will not be put back into the queue.
+ long timeoutMs = this.taskDispatchPolicy.getMaxTaskDispatchDuration().toMillis();
Review Comment:
> [#17796 (comment)](https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2623030826)
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2638551461
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterDispatchTimeoutCheckerConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch timeout checker.
+ * If enabled, tasks that remain in the dispatch queue longer than {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class MasterDispatchTimeoutCheckerConfig {
+
+ /**
+ * Whether to enable the dispatch timeout checking mechanism.
+ */
+ private boolean enabled = false;
+
+ /**
+ * Maximum allowed time for a task to be dispatched to a worker.
+ * Tasks exceeding this duration in the dispatch queue will be failed.
+ * Examples: "2m", "5m", "30m". Defaults to 5 minutes.
+ */
+ private Duration maxTaskDispatchDuration = Duration.ofMinutes(5);
Review Comment:
> When we don't have a good suggested value, don't set a default value, as this can mislead many users.
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2637534201
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
Review Comment:
It's better to use `taskExecutionRunnableId `, since the outer layer is unaware that this is the taskInstanceId.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2684645579
##########
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcherTest.java:
##########
@@ -46,7 +47,8 @@ class WorkerGroupDispatcherTest {
@BeforeEach
void setUp() {
taskExecutorClient = mock(ITaskExecutorClient.class);
- dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient);
+ final MasterConfig masterConfig = new MasterConfig();
+ dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient, masterConfig.getDispatchTimeout());
Review Comment:
> You should add a validated test case.
I've already validated and tested it in a real environment.
The LogicFakeTask type runs on the Master and does not involve WorkerGroup dispatching.
I track the fix in a separate issue: https://github.com/apache/dolphinscheduler/issues/17872
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3737629642
> @SbloodyS When you have time, could you please help trigger the CI pipeline? I'm unable to run the integration tests locally. Thank you so much!
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2719725674
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +93,69 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (Exception ex) {
+ if (taskDispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ // If a dispatch timeout occurs, the task will not be put back into the queue.
+ long timeoutMs = this.taskDispatchPolicy.getMaxTaskDispatchDuration().toMillis();
+ long elapsed = System.currentTimeMillis() - taskExecutionContext.getFirstDispatchTime();
+ if (elapsed > timeoutMs) {
+ handleDispatchFailure(taskExecutionRunnable, ex, elapsed, timeoutMs);
+ return;
+ }
+ }
+
// If dispatch failed, will put the task back to the queue
// The task will be dispatched after waiting time.
// the waiting time will increase multiple of times, but will not exceed 60 seconds
- long waitingTimeMills = Math.min(
+ long waitingTimeMillis = Math.min(
taskExecutionRunnable.getTaskExecutionContext().increaseDispatchFailTimes() * 1_000L, 60_000L);
- dispatchTask(taskExecutionRunnable, waitingTimeMills);
- log.error("Dispatch Task: {} failed will retry after: {}/ms", taskExecutionRunnable.getId(),
- waitingTimeMills, e);
+ dispatchTask(taskExecutionRunnable, waitingTimeMillis);
+ log.warn("Dispatch Task: {} failed will retry after: {}/ms", taskInstanceId,
+ waitingTimeMillis, ex);
+ }
+ }
+
+ /**
+ * Marks the specified task as fatally failed due to an unrecoverable dispatch error,such as timeout
+ * Once this method is called, the task is considered permanently failed and will not be retried.
+ */
+ private void handleDispatchFailure(ITaskExecutionRunnable taskExecutionRunnable, Exception ex,
Review Comment:
> onDispatchTimeout
better
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3798086989
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3788612153
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3753514558
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2638550852
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterDispatchTimeoutCheckerConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch timeout checker.
+ * If enabled, tasks that remain in the dispatch queue longer than {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class MasterDispatchTimeoutCheckerConfig {
Review Comment:
>
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2638787057
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/lifecycle/event/TaskFatalLifecycleEvent.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.engine.task.lifecycle.event;
+
+import org.apache.dolphinscheduler.server.master.engine.ILifecycleEventType;
+import org.apache.dolphinscheduler.server.master.engine.task.lifecycle.AbstractTaskLifecycleEvent;
+import org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType;
+import org.apache.dolphinscheduler.server.master.engine.task.runnable.ITaskExecutionRunnable;
+
+import java.util.Date;
+
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+
+@Data
+@Builder
+@AllArgsConstructor
+public class TaskFatalLifecycleEvent extends AbstractTaskLifecycleEvent {
+
+ private final ITaskExecutionRunnable taskExecutionRunnable;
+
+ private final Date endTime;
+
+ @Override
+ public ILifecycleEventType getEventType() {
+ return TaskLifecycleEventType.FATAL;
+ }
+
+ @Override
+ public String toString() {
+ return "TaskFatalLifecycleEvent{" +
+ "task=" + taskExecutionRunnable.getName() +
+ ", endTime=" + endTime +
+ '}';
+ }
+}
Review Comment:
> Please remove this.
ok, will publish TaskFailedLifecycleEvent
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2637533740
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
+ final long timeoutMs = this.dispatchTimeout.toMillis();
Review Comment:
You can translate the duration into milliseconds in the constructor.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3753525557
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2739395421
##########
dolphinscheduler-master/src/main/resources/application.yaml:
##########
@@ -111,6 +111,11 @@ master:
# Master max concurrent workflow instances, when the master's workflow instance count exceeds this value, master server will be marked as busy.
max-concurrent-workflow-instances: 2147483647
worker-group-refresh-interval: 5m
+ # Task dispatch timeout check (currently disabled).
+ # When enabled, tasks not dispatched within this duration are marked as failed.
+ task-dispatch-policy:
+ dispatch-timeout-enabled: false
+ max-task-dispatch-duration: 1h
Review Comment:
Please add to the doc.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3816024325
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2625693235
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -74,6 +74,12 @@ public class MasterConfig implements Validator {
*/
private String masterRegistryPath;
+ /**
+ * Maximum time allowed for a task to be successfully dispatched.
+ * Default: 5 minutes.
+ */
+ private Duration dispatchTimeout = Duration.ofMinutes(5);
Review Comment:
> maxTaskDispatchDuration
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2719730646
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +93,69 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (Exception ex) {
+ if (taskDispatchPolicy.isDispatchTimeoutFailedEnabled()) {
+ // If a dispatch timeout occurs, the task will not be put back into the queue.
+ long timeoutMs = this.taskDispatchPolicy.getMaxTaskDispatchDuration().toMillis();
+ long elapsed = System.currentTimeMillis() - taskExecutionContext.getFirstDispatchTime();
+ if (elapsed > timeoutMs) {
+ handleDispatchFailure(taskExecutionRunnable, ex, elapsed, timeoutMs);
+ return;
+ }
+ }
+
// If dispatch failed, will put the task back to the queue
// The task will be dispatched after waiting time.
// the waiting time will increase multiple of times, but will not exceed 60 seconds
- long waitingTimeMills = Math.min(
+ long waitingTimeMillis = Math.min(
taskExecutionRunnable.getTaskExecutionContext().increaseDispatchFailTimes() * 1_000L, 60_000L);
- dispatchTask(taskExecutionRunnable, waitingTimeMills);
- log.error("Dispatch Task: {} failed will retry after: {}/ms", taskExecutionRunnable.getId(),
- waitingTimeMills, e);
+ dispatchTask(taskExecutionRunnable, waitingTimeMillis);
+ log.warn("Dispatch Task: {} failed will retry after: {}/ms", taskInstanceId,
+ waitingTimeMillis, ex);
+ }
+ }
+
+ /**
+ * Marks the specified task as fatally failed due to an unrecoverable dispatch error,such as timeout
+ * Once this method is called, the task is considered permanently failed and will not be retried.
+ */
+ private void handleDispatchFailure(ITaskExecutionRunnable taskExecutionRunnable, Exception ex,
+ long elapsed, long timeoutMs) {
+ final String taskName = taskExecutionRunnable.getName();
+
+ log.warn("Dispatch fail, taskName: {}, timed out after {} ms (limit: {} ms))", taskName, elapsed, timeoutMs);
+
+ if (ExceptionUtils.isWorkerGroupNotFoundException(ex)) {
+ log.error("Dispatch fail, taskName: {}, Worker group not found.", taskName, ex);
+ final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
+ .taskExecutionRunnable(taskExecutionRunnable)
+ .endTime(new Date())
+ .build();
+ taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
+ } else if (ExceptionUtils.isNoAvailableWorkerException(ex)) {
+ log.error("Dispatch fail, taskName: {}, No available worker.", taskName, ex);
+ final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
+ .taskExecutionRunnable(taskExecutionRunnable)
+ .endTime(new Date())
+ .build();
+ taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
+ } else {
+ log.error("Dispatch fail, taskName: {}, Unexpected dispatch error.", taskName, ex);
+ final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
+ .taskExecutionRunnable(taskExecutionRunnable)
+ .endTime(new Date())
+ .build();
+ taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
Review Comment:
> log.error("Task: {} dispatch timeout.", taskName, ex);
> final TaskFailedLifecycleEvent taskFailedEvent = TaskFailedLifecycleEvent.builder()
> .taskExecutionRunnable(taskExecutionRunnable)
> .endTime(new Date())
> .build();
> taskExecutionRunnable.getWorkflowEventBus().publish(taskFailedEvent);
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3817175735
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS merged PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3831171066
After #17932 we may don't need to support dispatch timeout?
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3832466960
> #17932 could provide a more complete solution, effectively preventing abnormal tasks from consuming system resources.
I personally don't think there's a conflict here. The dispatch timeout shouldn't be very long—such exceptions should raise errors as early as possible (e.g., after 10 minutes). In contrast, your workflow or task execution timeout should be configured with a sufficiently long duration, as some weekly tasks may easily run for more than a day.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3832472833
> #17932 could provide a more complete solution, effectively preventing abnormal tasks from consuming system resources.
Dispatch timeout is intended to avoid wasting Master resources, while workflow or task timeout is primarily aimed at preventing Worker resource waste. Wouldn't it be better to have both?
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2626081271
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
+ final long timeoutMs = this.dispatchTimeout.toMillis();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (TaskDispatchException ex) {
+ // Checks whether the given task has exceeded its allowed dispatch timeout.
+ long elapsed = System.currentTimeMillis() - taskExecutionContext.getFirstDispatchEnqueueTimeMs();
+ if (elapsed > timeoutMs) {
+ handleDispatchFailure(taskExecutionRunnable, ex, elapsed, timeoutMs);
+ return;
+ }
+
// If dispatch failed, will put the task back to the queue
// The task will be dispatched after waiting time.
// the waiting time will increase multiple of times, but will not exceed 60 seconds
- long waitingTimeMills = Math.min(
+ long waitingTimeMillis = Math.min(
taskExecutionRunnable.getTaskExecutionContext().increaseDispatchFailTimes() * 1_000L, 60_000L);
- dispatchTask(taskExecutionRunnable, waitingTimeMills);
- log.error("Dispatch Task: {} failed will retry after: {}/ms", taskExecutionRunnable.getId(),
- waitingTimeMills, e);
+ dispatchTask(taskExecutionRunnable, waitingTimeMillis);
+ log.warn("Dispatch Task: {} failed will retry after: {}/ms", taskId,
+ waitingTimeMillis, ex);
+ }
+ }
+
+ /**
+ * Marks the specified task as fatally failed due to an unrecoverable dispatch error,such as timeout or persistent client failure.
+ * Once this method is called, the task is considered permanently failed and will not be retried.
+ *
+ * @param taskExecutionRunnable the task to mark as fatally failed; must not be null
+ * @param exception the dispatch exception that triggered this failure handling; must not be null
+ * @param elapsed the time (in milliseconds) already spent attempting to dispatch the task
+ * @param timeoutMs the configured dispatch timeout threshold (in milliseconds)
+ */
+ private void handleDispatchFailure(ITaskExecutionRunnable taskExecutionRunnable, TaskDispatchException exception,
Review Comment:
> Please don't print the `taskId` and `workflowId` here, all ids should already be added by MDC. We should only need to print the exception here, the `exception` already contains failure message.
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2626083591
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/exception/dispatch/WorkerNotFoundException.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.exception.dispatch;
+
+public class WorkerNotFoundException extends TaskDispatchException {
+
+ public WorkerNotFoundException(String message) {
Review Comment:
>
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3788605655
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2719171018
##########
dolphinscheduler-master/src/main/resources/application.yaml:
##########
@@ -111,6 +111,11 @@ master:
# Master max concurrent workflow instances, when the master's workflow instance count exceeds this value, master server will be marked as busy.
max-concurrent-workflow-instances: 2147483647
worker-group-refresh-interval: 5m
+ # Task dispatch timeout check (currently disabled).
+ # When enabled, tasks not dispatched within this duration are marked as failed.
+ task-dispatch-policy:
+ dispatch-timeout-failed-enabled: false
+ max-task-dispatch-duration: 5m
Review Comment:
We can remove the `max-task-dispatch-duration` here, since these is not used when `dispatch-timeout-failed-enabled` is false, only need to add these on the doc.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3798092089
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3816036655
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2621406378
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -74,6 +74,12 @@ public class MasterConfig implements Validator {
*/
private String masterRegistryPath;
+ /**
+ * Maximum time allowed for a task to be successfully dispatched.
+ * Default: 5 minutes.
+ */
+ private Duration dispatchTimeout = Duration.ofMinutes(5);
Review Comment:
We should make it configurable by users in the configuration file and it can be turned off.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2626082820
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/exception/dispatch/WorkerGroupNotFoundException.java:
##########
@@ -19,7 +19,7 @@
public class WorkerGroupNotFoundException extends TaskDispatchException {
- public WorkerGroupNotFoundException(String workerGroup) {
- super("Cannot find worker group: " + workerGroup);
+ public WorkerGroupNotFoundException(String message) {
Review Comment:
> Why do this change?
restore, remove the task info
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2625503768
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -74,6 +74,12 @@ public class MasterConfig implements Validator {
*/
private String masterRegistryPath;
+ /**
+ * Maximum time allowed for a task to be successfully dispatched.
+ * Default: 5 minutes.
+ */
+ private Duration dispatchTimeout = Duration.ofMinutes(5);
Review Comment:
> We should make it configurable by users in the configuration file and it can be turned off.
good idea!
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2638550477
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -74,6 +74,13 @@ public class MasterConfig implements Validator {
*/
private String masterRegistryPath;
+ /**
+ * Configuration for the master's task dispatch timeout check mechanism.
+ * This controls whether the system enforces a time limit for dispatching tasks to workers,
+ * and if so, how long to wait before marking a task as failed due to dispatch timeout.
+ */
+ private MasterDispatchTimeoutCheckerConfig dispatchTimeoutChecker = new MasterDispatchTimeoutCheckerConfig();
Review Comment:
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2641705701
##########
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcherTest.java:
##########
@@ -138,4 +156,285 @@ void dispatch_TaskDispatchFails_RetryLogicWorks() throws TaskDispatchException {
.untilAsserted(() -> verify(taskExecutorClient, times(2)).dispatch(taskExecutionRunnable));
}
+ @Test
Review Comment:
> You should add UT or IT test case to verify the time out check is enabled. Your core logic has not been tested.
OK. I need to modify the master's configuration. I'll submit the changes and test them — I can't run the integration tests locally.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2626084354
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/TaskExecutionContext.java:
##########
@@ -126,6 +126,11 @@ public class TaskExecutionContext implements Serializable {
private boolean failover;
+ /**
+ * Timestamp (ms) when the task was first enqueued for dispatch.
+ */
+ private final long firstDispatchEnqueueTimeMs = System.currentTimeMillis();
Review Comment:
> This is more like the creation time of the `taskExecutionContext`.
ok
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2626772850
##########
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcherTest.java:
##########
@@ -46,7 +47,8 @@ class WorkerGroupDispatcherTest {
@BeforeEach
void setUp() {
taskExecutorClient = mock(ITaskExecutorClient.class);
- dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient);
+ final MasterConfig masterConfig = new MasterConfig();
+ dispatcher = new WorkerGroupDispatcher("TestGroup", taskExecutorClient, masterConfig.getDispatchTimeout());
Review Comment:
> You should add a validated test case.
ok, add test for dispatch timeout checker
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2637537371
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterDispatchTimeoutCheckerConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch timeout checker.
+ * If enabled, tasks that remain in the dispatch queue longer than {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class MasterDispatchTimeoutCheckerConfig {
+
+ /**
+ * Whether to enable the dispatch timeout checking mechanism.
+ */
+ private boolean enabled = false;
+
+ /**
+ * Maximum allowed time for a task to be dispatched to a worker.
+ * Tasks exceeding this duration in the dispatch queue will be failed.
+ * Examples: "2m", "5m", "30m". Defaults to 5 minutes.
+ */
+ private Duration maxTaskDispatchDuration = Duration.ofMinutes(5);
Review Comment:
```suggestion
private Duration maxTaskDispatchDuration;
```
When we don't have a good suggested value, don't set a default value, as this can mislead many users.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterDispatchTimeoutCheckerConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch timeout checker.
+ * If enabled, tasks that remain in the dispatch queue longer than {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class MasterDispatchTimeoutCheckerConfig {
Review Comment:
```suggestion
public class TaskDispatchPolicy {
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterDispatchTimeoutCheckerConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.config;
+
+import java.time.Duration;
+
+import lombok.Data;
+
+/**
+ * Configuration for the master's task dispatch timeout checker.
+ * If enabled, tasks that remain in the dispatch queue longer than {@link #maxTaskDispatchDuration} will be marked as failed to prevent indefinite queuing.
+ */
+@Data
+public class MasterDispatchTimeoutCheckerConfig {
+
+ /**
+ * Whether to enable the dispatch timeout checking mechanism.
+ */
+ private boolean enabled = false;
Review Comment:
```suggestion
private boolean dispatchTimeoutFailedEnabled = false;
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/lifecycle/event/TaskFatalLifecycleEvent.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.dolphinscheduler.server.master.engine.task.lifecycle.event;
+
+import org.apache.dolphinscheduler.server.master.engine.ILifecycleEventType;
+import org.apache.dolphinscheduler.server.master.engine.task.lifecycle.AbstractTaskLifecycleEvent;
+import org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType;
+import org.apache.dolphinscheduler.server.master.engine.task.runnable.ITaskExecutionRunnable;
+
+import java.util.Date;
+
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+
+@Data
+@Builder
+@AllArgsConstructor
+public class TaskFatalLifecycleEvent extends AbstractTaskLifecycleEvent {
+
+ private final ITaskExecutionRunnable taskExecutionRunnable;
+
+ private final Date endTime;
+
+ @Override
+ public ILifecycleEventType getEventType() {
+ return TaskLifecycleEventType.FATAL;
+ }
+
+ @Override
+ public String toString() {
+ return "TaskFatalLifecycleEvent{" +
+ "task=" + taskExecutionRunnable.getName() +
+ ", endTime=" + endTime +
+ '}';
+ }
+}
Review Comment:
Please remove this.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,74 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskInstanceId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
try {
- if (!waitingDispatchTaskIds.remove(taskExecutionRunnable.getId())) {
+ if (!waitingDispatchTaskIds.remove(taskInstanceId)) {
log.info(
"The task: {} doesn't exist in waitingDispatchTaskIds(it might be paused or killed), will skip dispatch",
- taskExecutionRunnable.getId());
+ taskInstanceId);
return;
}
taskExecutorClient.dispatch(taskExecutionRunnable);
- } catch (Exception e) {
+ } catch (TaskDispatchException ex) {
Review Comment:
```suggestion
} catch (Exception ex) {
```
Currently, at least in this PR, you can't guarantee that other exceptions won't be thrown here, right?
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterConfig.java:
##########
@@ -74,6 +74,13 @@ public class MasterConfig implements Validator {
*/
private String masterRegistryPath;
+ /**
+ * Configuration for the master's task dispatch timeout check mechanism.
+ * This controls whether the system enforces a time limit for dispatching tasks to workers,
+ * and if so, how long to wait before marking a task as failed due to dispatch timeout.
+ */
+ private MasterDispatchTimeoutCheckerConfig dispatchTimeoutChecker = new MasterDispatchTimeoutCheckerConfig();
Review Comment:
```suggestion
private TaskDispatchPolicy taskDispatchPolicy = new TaskDispatchPolicy();
```
##########
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcherTest.java:
##########
@@ -138,4 +156,285 @@ void dispatch_TaskDispatchFails_RetryLogicWorks() throws TaskDispatchException {
.untilAsserted(() -> verify(taskExecutorClient, times(2)).dispatch(taskExecutionRunnable));
}
+ @Test
Review Comment:
You should add UT or IT test case to verify the time out check is enabled. Your core logic has not been tested.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2625701912
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
+ final TaskExecutionContext taskExecutionContext = taskExecutionRunnable.getTaskExecutionContext();
+ final long timeoutMs = this.dispatchTimeout.toMillis();
Review Comment:
> It's better to store the mills rather than store duration, then you can avoid execute `dispatchTimeout.toMillis()` here.
Using a duration format like '5m' is more concise and readable. Most other configurations I’ve seen also avoid millisecond-level granularity.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master]Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2625708282
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/dispatcher/WorkerGroupDispatcher.java:
##########
@@ -84,23 +95,77 @@ public void run() {
}
private void doDispatchTask(ITaskExecutionRunnable taskExecutionRunnable) {
+ final int taskId = taskExecutionRunnable.getId();
Review Comment:
> taskExecutionRunnableId
final int taskInstanceId = taskExecutionRunnable.getId(); is it better?
/**
* Get the task instance id.
* <p> Need to know the id might change since the task instance might be regenerated.
*/
default int getId() {
return getTaskInstance().getId();
}
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "njnu-seafish (via GitHub)" <gi...@apache.org>.
njnu-seafish commented on code in PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#discussion_r2739665469
##########
dolphinscheduler-master/src/main/resources/application.yaml:
##########
@@ -111,6 +111,11 @@ master:
# Master max concurrent workflow instances, when the master's workflow instance count exceeds this value, master server will be marked as busy.
max-concurrent-workflow-instances: 2147483647
worker-group-refresh-interval: 5m
+ # Task dispatch timeout check (currently disabled).
+ # When enabled, tasks not dispatched within this duration are marked as failed.
+ task-dispatch-policy:
+ dispatch-timeout-enabled: false
+ max-task-dispatch-duration: 1h
Review Comment:
> Please add to the doc.
ok, add it.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3817163338
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=17796&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=17796&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=17796)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement-17795][Master] Add dispatch timeout checking logic to handle cases where the worker group does not exist or no workers are available [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #17796:
URL: https://github.com/apache/dolphinscheduler/pull/17796#issuecomment-3833481975
We can keep the both config.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org