[Improvement-18443][API&DAO] Optimize WorkflowInstanceMapper to exclude large text fields from list queries - #18444
[Improvement-18443][API&DAO] Optimize WorkflowInstanceMapper to exclude large text fields from list queries#18444njnu-seafish wants to merge 12 commits into
Conversation
|
The CI pipeline shows the following error: But it runs successfully on my local machine every time. |
@SbloodyS This error seems to be a frontend issue during the E2E test and is likely unrelated to my changes. Could you please help re-trigger the CI? Thanks a lot!
|
|
CI has been reruned. @njnu-seafish |
SbloodyS
left a comment
SbloodyS
left a comment
There was a problem hiding this comment.
public API response contract is unintentionally changed
queryWorkflowInstanceListPaging, queryTopNWorkflowInstance, and queryByTriggerCode now use listSql, but their results are returned directly by public API endpoints as WorkflowInstance objects.
Consequently, these previously populated response properties will become null or be omitted:
commandParamglobalParamshistoryCmdvarPoolstateHistory
This is therefore not only an internal DAO optimization—it changes the existing API response contract and may break API consumers.
Please either:
- Keep using
baseSqlfor queries whose entities are returned directly by public APIs; or - Introduce an explicit lightweight response DTO, document the incompatible API change, and add controller-level response regression tests.
The critical runtime queries reverted to baseSql look correct, but this public API compatibility issue remains blocking.
Exactly. The reviewer's points are incredibly thorough. I will go with Option 2. This allows us to retain the performance optimizations of listSql while introducing explicit, lightweight DTOs to lock down the API contract. We will no longer let entity fields implicitly dictate the API response. |
…ntentionally omit commandParam/globalParams/historyCmd/varPool/stateHistory
|
@SbloodyS Could you please help trigger the CI pipeline and review the code? Thanks! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| instance.getHost(), | ||
| instance.getCommandType(), | ||
| instance.getTaskDependType(), | ||
| instance.getMaxTryTimes(), |
| instance.getTimeout(), | ||
| instance.getTenantCode(), | ||
| instance.getDryRun(), | ||
| instance.getNextWorkflowInstanceId(), |
| String executorName, | ||
| WorkflowExecutionStatus stateType, | ||
| String host, | ||
| String otherParamsJson, |
The DTO removes more response properties than the documented five
The affected endpoints previously returned Therefore, the actual incompatible response change is broader than what the upgrade document and DTO documentation describe. In particular, the complement-related properties can contain meaningful values for complement-data executions. Please either preserve all non-targeted response properties or document the complete set of removed properties. The regression tests should serialize an actual controller response and assert its JSON shape; the current reflection-only tests do not verify the public API response. Declare the DTO type on the primary list endpoint
The service now returns Please change the controller return type to |
All 13 removed properties have been documented (previously only 5 were listed). Three categories: Regarding complement properties: The frontend identifies complement-data instances via row.commandType === 'COMPLEMENT_DATA', not cmdTypeIfComplement/complementData. Since commandType is preserved, the frontend is unaffected. Changes made: @SbloodyS Could you please help trigger the CI pipeline and review the code? Thanks! |
# Conflicts: # docs/docs/en/guide/upgrade/incompatible.md # docs/docs/zh/guide/upgrade/incompatible.md
| * **Removed heavy fields**: `commandParam`, `globalParams`, `historyCmd`, `varPool`, `stateHistory` | ||
| * **Removed transient fields**: `stateDescList`, `workflowDefinition`, `dagData`, `queue`, `locations`, `dependenceScheduleTimes` | ||
| * **Removed derived properties**: `cmdTypeIfComplement`, `complementData` (related to complement-data executions; use the detail API to obtain them) | ||
| * To obtain any of these fields, use the detail API `GET /projects/{projectCode}/workflow-instances/{id}` instead, which continues to return the full `WorkflowInstance` object. ([#18444])(https://github.com/apache/dolphinscheduler/pull/18444) |
There was a problem hiding this comment.
| * To obtain any of these fields, use the detail API `GET /projects/{projectCode}/workflow-instances/{id}` instead, which continues to return the full `WorkflowInstance` object. ([#18444])(https://github.com/apache/dolphinscheduler/pull/18444) | |
| * To obtain any of these fields, use the detail API `GET /projects/{projectCode}/workflow-instances/{id}` instead, which continues to return the full `WorkflowInstance` object. ([#18444](https://github.com/apache/dolphinscheduler/pull/18444)) |
There was a problem hiding this comment.
ok. done.
@SbloodyS Could you please help trigger the CI pipeline and review the code? Thanks!
| * **移除的大字段**：`commandParam`、`globalParams`、`historyCmd`、`varPool`、`stateHistory` | ||
| * **移除的非数据库字段**：`stateDescList`、`workflowDefinition`、`dagData`、`queue`、`locations`、`dependenceScheduleTimes` | ||
| * **移除的派生属性**：`cmdTypeIfComplement`、`complementData`（补数执行相关，如需获取请使用详情接口） | ||
| * 如需获取这些字段，请使用详情接口 `GET /projects/{projectCode}/workflow-instances/{id}`，该接口仍返回完整的 `WorkflowInstance` 对象 ([#18444])(https://github.com/apache/dolphinscheduler/pull/18444) |
There was a problem hiding this comment.
| * 如需获取这些字段，请使用详情接口 `GET /projects/{projectCode}/workflow-instances/{id}`，该接口仍返回完整的 `WorkflowInstance` 对象 ([#18444])(https://github.com/apache/dolphinscheduler/pull/18444) | |
| * 如需获取这些字段，请使用详情接口 `GET /projects/{projectCode}/workflow-instances/{id}`，该接口仍返回完整的 `WorkflowInstance` 对象 ([#18444](https://github.com/apache/dolphinscheduler/pull/18444)) |
3 participants
Was this PR generated or assisted by AI?
YES. Leverage LLMs to verify that the removed large text fields are indeed unused.
Purpose of the pull request
close #18443
Brief change log
The WorkflowInstanceMapper was returning all fields including large text/longtext fields (command_param, global_params, history_cmd, var_pool, state_history) for all queries,which caused unnecessary database I/O, network transfer, and memory usage. This commit adds a dedicated listSql SQL fragment that excludes these large fields, and updates the following list query methods to use them:
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
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