Repository navigation
[Fix-18672][UI] Allow SeaTunnel workflow tasks to select stream execution - #18673
sunlishuo25 wants to merge 1 commit into
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md) |
SbloodyS
left a comment
There was a problem hiding this comment.
The new E2E covers execution-type selection and save/reload, but leaves several changed paths untested.
Please add regression coverage for copying a STREAM SeaTunnel task, updating connected-edge styling when switching between BATCH and STREAM, and disabling SeaTunnel savepoint while keeping Flink savepoint enabled.
The test also never executes the workflow or checks the Stream Task instance list, so it does not verify the instance-classification outcome described in the PR.
|
Thanks for the review, @SbloodyS. Commit f1acc46 now includes regression coverage for STREAM task copying and persistence, connected-edge styling when switching BATCH/STREAM, disabled SeaTunnel savepoint with Flink savepoint preserved, and actual workflow execution followed by Stream/Batch instance-list checks. Local validation passed all 43 browser checks and the complete WorkflowSeaTunnelE2ETest class. The workflow was actually started, but the SeaTunnel tasks failed because /bin/seatunnel.sh was missing (exit 127). The STREAM classification assertions passed; this does not demonstrate successful SeaTunnel engine execution. Local validation used ARM64 with reduced JVM heap settings. The tests are in WorkflowSeaTunnelE2ETest.java and dolphinscheduler-ui/tests/task-execution-graph.cjs. The latter is included in the Frontend workflow. All nine upstream pull_request workflows for this head are still awaiting approval with zero jobs executed. |
| echo "Code format check failed! Please run \`pnpm run lint\` to format the code." | ||
| exit -1 | ||
| fi | ||
| - name: Task execution type regressions |
There was a problem hiding this comment.
Removed the extra step from frontend.yml in 1f47241; the existing lint/build checks are kept.
| import org.openqa.selenium.support.ui.ExpectedConditions; | ||
|
|
||
| @DolphinScheduler(composeFiles = "docker/basic/docker-compose.yaml") | ||
| class WorkflowSeaTunnelE2ETest { |
There was a problem hiding this comment.
This is a fake e2e test. The principle of e2e is to verify the real user scenario, which did not start the seatunnel task.
There was a problem hiding this comment.
The test now uses the real SeaTunnel 2.3.3 fixture in docker/seatunnel-task. Two complete local runs of 1f47241 passed: Console output, UI Stop, the same PID exiting, and a successful BATCH run. The local setup differences and pending upstream approval are described in the updated PR body.
| return null; | ||
| } | ||
| // Require an unclipped center so the hit test matches the native pointer location. | ||
| boolean target = Boolean.TRUE.equals(((JavascriptExecutor) driver).executeScript( |
SbloodyS
left a comment
There was a problem hiding this comment.
WorkflowSeaTunnelE2ETest failed. Please fix it. @sunlishuo25
|
@SbloodyS I ran the full WorkflowSeaTunnelE2ETest locally, and it passed on Linux ARM64 with the default application and SeaTunnel heap settings: 1 test, 0 failures, 0 errors, 0 skipped. I've attached the test report, relevant logs, and recording. |
| needs: build | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| timeout-minutes: ${{ matrix.case.name == 'WorkflowSeaTunnelE2ETest' && 90 || 30 }} |
There was a problem hiding this comment.
All e2e tests need to be completed within 30 minutes. Please optimize the implementation.
|
@SbloodyS I've restored the 30-minute timeout and fixed the E2E UI interactions. Both WorkflowSwitchE2ETest and WorkflowSeaTunnelE2ETest passed locally on Linux ARM64 with the default application and SeaTunnel heap settings. The test reports, logs, and recordings are attached in one ZIP. |
| public final class SwitchTaskForm extends TaskNodeForm { | ||
|
|
||
| @FindBy(id = "btnAddIfBranch") | ||
| static final String NODE_MODAL = "//div[contains(concat(' ', normalize-space(@class), ' '), ' n-modal ')" |
There was a problem hiding this comment.
Please don't modify irrelevant code in all places.
There was a problem hiding this comment.
Removed the unrelated changes and pushed the update. The local SeaTunnel E2E test passed; CI is awaiting approval.
|
@SbloodyS I've cleaned up the PR in 850ce3e:
The same commit passed the official E2E workflow on my fork: WorkflowSeaTunnelE2ETest (1 test, 0 failures, ~7 min on AMD64), and all 24 E2E jobs were green. |
|
@SbloodyS I've resolved the conflicts by merging the latest This update also covers your earlier request to keep Flink savepoint enabled. On my fork, the E2E workflow for this commit passed: all 24 jobs after re-running one unrelated job whose Docker Compose environment failed to start. Backend, Frontend, Test, API-Test, E2E-K8S and CodeQL passed as well. |
SbloodyS
left a comment
There was a problem hiding this comment.
Please refactor this E2E test to follow the project's existing Page Object Model and test structure.
Reuse the existing Page Objects for project navigation, task configuration, submission, and workflow execution. If an existing interaction needs improvement, fix it in the corresponding Page Object instead of adding a separate implementation inside WorkflowSeaTunnelE2ETest.
Please also consolidate overlapping node lookup/opening logic, reduce unnecessary helper methods, and keep changes to shared helpers such as addTask and dragAndDrop.js limited to demonstrated needs. Split the long test into focused scenarios with explicit setup dependencies, while preserving the real SeaTunnel/Flink execution and regression assertions.
|
|
||
| @SneakyThrows | ||
| @SuppressWarnings("unchecked") | ||
| public <T> T addTask(TaskType type, Integer x, Integer y) { |
There was a problem hiding this comment.
Removed the overload and restored dragAndDrop.js to dev. The test now lays out the dropped nodes with the existing formatDAG() instead of passing drop coordinates; I fixed formatDAG() / WorkflowFormatDialog because they no longer matched the current UI (details in my review reply).
|
@SbloodyS Thanks for the review. I've refactored the test to use the existing page objects: it is now Where the existing page objects failed, and the fix:
The other page object additions are new methods the scenarios need. CI runs on my fork:
|
SbloodyS
left a comment
There was a problem hiding this comment.
Please make sure you understand and can explain every change before submitting AI-assisted code.
This PR has gone through several rounds of adding helpers, replacing existing implementations, and removing code. Please review the final diff yourself against the project's existing E2E structure, justify each change to shared code, and verify that existing behavior and coverage are preserved.
AI assistance is welcome, but generated code and passing CI do not replace understanding the implementation. Please keep the changes focused and submit only code you have personally reviewed and can maintain. Before that, I won't do the next review.
|
@SbloodyS I found this issue while deploying DolphinScheduler with SeaTunnel at my company. I fixed it locally with the help of AI, and SeaTunnel tasks ran correctly in both batch and streaming mode in our environment, so I opened this PR. I didn't consider DolphinScheduler's overall architecture and the existing E2E structure carefully enough. Thank you for the reviews and your patience throughout this process, and all the best with your work. When I have time, I'll continue contributing to the community. |
|
As a first time contributor for this bugfix, you can remove all the codes related to e2e, only including the fixed codes. @sunlishuo25 |
64e5cac to
f3c66eb
Compare
| { | ||
| type: 'select', | ||
| field: 'taskExecuteType', | ||
| span: 12, | ||
| name: t('project.node.task_execute_type'), | ||
| options: [ | ||
| { label: t('project.task.batch_task'), value: 'BATCH' }, | ||
| { label: t('project.task.stream_task'), value: 'STREAM' } | ||
| ], | ||
| validate: { | ||
| trigger: ['input', 'blur'], | ||
| required: true | ||
| } | ||
| }, |
There was a problem hiding this comment.
SeaTunnel can run either batch or streaming jobs (job.mode in the config), but taskExecuteType was always defaulted to BATCH by use-task.ts, because only FLINK_STREAM is marked as STREAM in TASK_TYPES_MAP. This select lets the user set it explicitly.
- It is added to the
useSeaTunnellist like the other SeaTunnel params, sofields/index.tsand the task form list are unchanged. - Rendering and the required check reuse the common
selectfield andvalidaterule, the same as Worker group. - No
valueis set here on purpose: the defaultBATCHstill comes fromuse-task.ts, and when an existing task is opened,detail-modal.tsxrestores the saved value instead of overwriting it.
| minute: '分', | ||
| second: '秒', | ||
| delay_execution_time: '延时执行时间', | ||
| task_execute_type: '任务执行类型', |
There was a problem hiding this comment.
New label for the select. The two options reuse the existing project.task.batch_task (批量任务) and project.task.stream_task (实时任务), so they match the Batch/Stream tabs on the Task Instance page.
| minute: 'Minute', | ||
| second: 'Second', | ||
| delay_execution_time: 'Delay execution time', | ||
| task_execute_type: 'Task execution type', |
There was a problem hiding this comment.
English label for the select. The options reuse the existing Batch Task / Stream Task labels.
|
@SbloodyS Thanks for the review. Over the past few days I went through the SeaTunnel-related code and tried to keep the change as small as possible. I removed all the E2E changes and kept only the fix itself. It now changes just the SeaTunnel task form and the two locale files (3 files, +16 lines). I left a comment on each change to explain it. Thanks! |
SbloodyS
left a comment
There was a problem hiding this comment.
Disable the unsupported Savepoint action for SeaTunnel
[Evidence] Selecting STREAM causes SeaTunnel instances to appear in the Stream Task list, where the Savepoint action is displayed unconditionally.
[Evidence] SeatunnelTask does not implement StreamTask, so the worker returns The taskInstance is not StreamTask. The API currently ignores that failed response and still returns SUCCESS.
[Impact] Users may receive a successful Savepoint notification even though no savepoint was created.
[Suggestion] Disable the Savepoint action for SeaTunnel tasks. Alternatively, implement the complete StreamTask contract and propagate the worker failure to the API response.
Preserve the selected execution type in DAG metadata
[Evidence] The new field persists taskExecuteType in the task definition, but DAG nodes and edges still derive it from TASK_TYPES_MAP[type].taskExecuteType.
[Evidence] SEATUNNEL has no static STREAM marker in that map, and taskConfirm() does not update the node's taskExecuteType after the form is saved.
[Impact] After switching a SeaTunnel task to STREAM, connected edges are still rendered as batch edges. The same incorrect styling remains after reopening the workflow.
[Suggestion] Build and update DAG node metadata from the task definition's dynamic taskExecuteType, then recalculate the styles of all connected edges.
Was this PR generated or assisted by AI?
YES. I found this issue myself. I used AI as an assistant to help write the code changes, and I manually reviewed and verified all of them.
Purpose of the pull request
Fixes #18672.
A SeaTunnel task is always saved as Batch, because the form has no way to choose otherwise, so a streaming SeaTunnel job never shows up under Task Instance -> Stream Task.
This PR adds a required "Task execution type" select to the SeaTunnel task form, with only two options: Batch Task / Stream Task. Batch stays the default, so existing tasks are not affected.
Brief change log
en_US/zh_CNlocale files: add one label line each. The two options reuse the existing labels.Verify this pull request
Manually tested on an environment with SeaTunnel 2.3.3:
Pull Request Notice
Pull Request Notice