Copilot commented on code in PR #17821:
URL:
https://github.com/apache/dolphinscheduler/pull/17821#discussion_r2650132700
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/statemachine/TaskSubmittedStateAction.java:
##########
@@ -109,7 +110,14 @@ public void onDispatchEvent(final
IWorkflowExecutionRunnable workflowExecutionRu
taskInstance.getDelayTime(),
remainTimeMills);
}
- taskExecutionRunnable.initializeTaskExecutionContext();
+
+ try {
+ taskExecutionRunnable.initializeTaskExecutionContext();
+ } catch (Exception ex) {
+ log.error("Failed to initialize task execution context, taskName:
{}", taskInstance.getName(), ex);
+ throw new TaskExecutionContextCreateException(ex.getMessage());
+
Review Comment:
Unnecessary blank line after the throw statement. This extra blank line
should be removed to maintain consistent code formatting.
```suggestion
```
##########
dolphinscheduler-master/src/test/resources/it/start/workflow_with_one_forbidden_condition_task_with_one_fake_predecessor_fatal.yaml:
##########
@@ -0,0 +1,125 @@
+#
+# 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.
+#
+
+# A(failed) -> B(Condition)(forbidden) -> C(success)
+# -> D(failed)
+project:
+ name: MasterIntegrationTest
+ code: 1
+ description: This is a fake project
+ userId: 1
+ userName: admin
+ createTime: 2024-08-12 00:00:00
+ updateTime: 2024-08-12 00:00:00
+
+workflows:
+ - name:
workflow_with_one_forbidden_condition_task_with_one_fake_predecessor_fatal
+ code: 1
+ version: 1
+ projectCode: 1
+ description: This is a fake workflow with one forbidden condition task
which has one predecessor fatal
Review Comment:
The description reads awkwardly. It should be "This is a fake workflow with
one forbidden condition task that has one predecessor that failed fatally" or
"...with one predecessor that encountered a fatal error". The current phrasing
"has one predecessor fatal" is grammatically incorrect.
```suggestion
description: This is a fake workflow with one forbidden condition task
that has one predecessor that failed fatally
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/WorkflowEventBusFireWorker.java:
##########
@@ -128,6 +131,19 @@ private void doFireSingleWorkflowEventBus(final
IWorkflowExecutionRunnable workf
ThreadUtils.sleep(5_000);
return;
}
+
+ // If task initializeTaskExecutionContext before dispatch is
failed
+ // construct and publish a dedicated TaskFatalLifecycleEvent
+ // so that the event will be handled by
TaskFatalLifecycleEventHandler
+ if (ExceptionUtils.isTaskExecutionContextCreateException(ex)) {
+ AbstractTaskLifecycleEvent taskLifecycleEvent =
(AbstractTaskLifecycleEvent) lifecycleEvent;
+ final TaskFatalLifecycleEvent taskFatalEvent =
TaskFatalLifecycleEvent.builder()
+
.taskExecutionRunnable(taskLifecycleEvent.getTaskExecutionRunnable())
+ .endTime(new Date())
+ .build();
+ workflowEventBus.publish(taskFatalEvent);
Review Comment:
Unsafe type casting without verification. While the
TaskExecutionContextCreateException is currently only thrown when handling
TaskDispatchLifecycleEvent (which extends AbstractTaskLifecycleEvent), it would
be safer to check the type before casting to prevent potential
ClassCastException if the code structure changes. Consider adding an instanceof
check before the cast.
```suggestion
if (lifecycleEvent instanceof
AbstractTaskLifecycleEvent) {
AbstractTaskLifecycleEvent taskLifecycleEvent =
(AbstractTaskLifecycleEvent) lifecycleEvent;
final TaskFatalLifecycleEvent taskFatalEvent =
TaskFatalLifecycleEvent.builder()
.taskExecutionRunnable(taskLifecycleEvent.getTaskExecutionRunnable())
.endTime(new Date())
.build();
workflowEventBus.publish(taskFatalEvent);
} else {
log.warn(
"TaskExecutionContextCreateException
occurred for non-task lifecycle event: {}",
lifecycleEvent.getClass().getName(),
ex);
}
```
##########
dolphinscheduler-master/src/test/resources/it/start/workflow_with_one_condition_task_with_one_fake_predecessor_fatal.yaml:
##########
@@ -0,0 +1,123 @@
+#
+# 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.
+#
+
+# A(fatal-failed) -> B(success) -> D(success)
+project:
+ name: MasterIntegrationTest
+ code: 1
+ description: This is a fake project
+ userId: 1
+ userName: admin
+ createTime: 2024-08-12 00:00:00
+ updateTime: 2024-08-12 00:00:00
+
+workflows:
+ - name: workflow_with_one_condition_task_with_one_fake_predecessor_fatal
+ code: 1
+ version: 1
+ projectCode: 1
+ description: This is a fake workflow with one condition task which has one
predecessor fatal
Review Comment:
The description reads awkwardly. It should be "This is a fake workflow with
one condition task that has one predecessor that failed fatally" or "...with
one predecessor that encountered a fatal error". The current phrasing "has one
predecessor fatal" is grammatically incorrect.
```suggestion
description: This is a fake workflow with one condition task that has
one predecessor that failed fatally
```
##########
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;
Review Comment:
This method overrides
[AbstractTaskLifecycleEvent.getTaskExecutionRunnable](1); it is advisable to
add an Override annotation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
