[Feature-17501][Worker] Generic Third-party System API Connector for REST-based Task Scheduling #17779
Conversation
dill21yu
requested review from
EricGao888,
Gallardot,
SbloodyS,
caishunfeng,
ruanwenjun and
songjianet
as code owners
dill21yu
changed the title
dill21yu
changed the title
…o thirdparty-system-connector
|
There was a problem hiding this comment.
Pull request overview
This pull request implements a Generic Third-party System API Connector for REST-based task scheduling in DolphinScheduler. The feature allows users to configure and schedule tasks on external systems through REST APIs with support for authentication (Basic Auth, OAuth2, JWT), task submission, status polling, and cancellation.
Key Changes:
- New UI module for managing third-party API sources with form validation and modal dialogs
- Task plugin implementation for executing external system tasks with retry logic and timeout handling
- Datasource plugin for third-party system connector configuration
- Internationalization support (English and Chinese)
Reviewed changes
Copilot reviewed 73 out of 76 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| dolphinscheduler-ui/src/views/thirdparty-api-source/* | New UI components for managing third-party API sources including modal forms, table views, and styling |
| dolphinscheduler-ui/src/locales/*/thirdparty-api-source.ts | Internationalization strings for the new feature in Chinese and English |
| dolphinscheduler-task-plugin/dolphinscheduler-task-external-system/* | Task plugin implementation with HTTP client logic, authentication, and status polling |
| dolphinscheduler-ui/src/store/project/* | Task type definitions and additions for external system tasks |
| dolphinscheduler-spi/src/main/java/.../DbType.java | New database type enum for third-party system connector |
| dolphinscheduler-ui/src/views/datasource/list/* | UI components for datasource management with third-party system support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { | ||
| queryDataSourceListPaging, | ||
| deleteDataSource | ||
| } from '@/service/modules/data-source' |
There was a problem hiding this comment.
The imports reference data-source module but this file is in the thirdparty-api-source directory. This creates a dependency on the wrong module. The file should either use dedicated third-party API source service methods or be moved to use the correct service module for consistency.
| taskExecuteType: 'STREAM' | ||
| }, | ||
|
|
||
| NAL_SYSTEM : { |
There was a problem hiding this comment.
The property name 'NAL_SYSTEM' appears to be incomplete or corrupted. This should be 'EXTERNAL_SYSTEM' to match the task type definition elsewhere in the codebase.
| &.icon-external_stream { | ||
| background-image: url('/images/task-icons/external_stream_hover.png'); | ||
| } |
There was a problem hiding this comment.
The CSS class name 'icon-external_stream' doesn't match the task type 'EXTERNAL_SYSTEM'. This should be 'icon-external_system' for consistency with the icon defined at line 120.
| if (headers == null || !headers.containsKey(ExternalTaskConstants.CONTENT_TYPE) | ||
| || !headers.containsKey(ExternalTaskConstants.CONTENT_TYPE_LOWERCASE)) { | ||
| return OkHttpRequestHeaderContentType.APPLICATION_JSON; |
There was a problem hiding this comment.
The condition uses OR (||) instead of AND (&&) when checking for missing Content-Type headers. This means the function returns APPLICATION_JSON even when one of the headers exists. The logic should be: if headers is null OR (does not contain CONTENT_TYPE AND does not contain CONTENT_TYPE_LOWERCASE).
| long usedTime = (usedTimeMillis + 29999) / 60000; // Over 30 seconds takes 1 minute, less than 30 | ||
| // seconds takes 0 minutes | ||
| log.info( | ||
| "External task timeout check, used time: {}m, timeout: {}m, currentTime: {}, taskStartTime: {}", | ||
| usedTime, taskExecutionContext.getTaskTimeout() / 60, currentTime, taskStartTime); |
There was a problem hiding this comment.
The timeout calculation uses an unusual formula (usedTimeMillis + 29999) / 60000 which rounds up to minutes. The comment says "Over 30 seconds takes 1 minute, less than 30 seconds takes 0 minutes" but this formula actually rounds times between 0-30 seconds to 0 minutes, and times between 30-90 seconds to 1 minute. This may not match the intended timeout behavior. Consider using a standard rounding approach or clarifying the intended behavior.
| import { | ||
| ThirdpartyApiSourceReq, | ||
| ThirdpartyApiSource, | ||
| } from './types' |
There was a problem hiding this comment.
Unused import ThirdpartyApiSource.
| } | ||
|
|
||
| const deleteRecord = async (id: number) => { | ||
| const ignored = await deleteDataSource(id) |
There was a problem hiding this comment.
Unused variable ignored.
| NTooltip | ||
| } from 'naive-ui' | ||
| import { useI18n } from 'vue-i18n' | ||
| import { useRouter } from 'vue-router' |
There was a problem hiding this comment.
Unused import useRouter.
| /> | ||
| <NSpace justify='center'> | ||
| <NPagination | ||
| page={page.value} |
There was a problem hiding this comment.
This write to property 'page' is useless, since another property write always overrides it.
| <NSpace justify='center'> | ||
| <NPagination | ||
| page={page.value} | ||
| page-size={pageSize.value} |
There was a problem hiding this comment.
This write to property 'page-size' is useless, since another property write always overrides it.
2 participants
Purpose of the pull request
to close #17501
Pull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.md