njnu-seafish commented on PR #18549: URL: https://github.com/apache/dolphinscheduler/pull/18549#issuecomment-5887650984
> ### Keep the log display limit separate from alert content. > In [SqlTask.java, lines 337–344](https://github.com/apache/dolphinscheduler/blob/872fc5095db90479cbfffac8839d844c1ce325fa/dolphinscheduler-task-plugin/dolphinscheduler-task-sql/src/main/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTask.java#L337-L344), displayRows now truncates the notification payload. The UI labels this setting “Log display,” and the previous implementation passed the complete result to the alert. With 50 result rows and displayRows=10, recipients now receive only 10 rows without any truncation notice. Please preserve the full alert result or introduce a separate, explicit alert limit. > > ### The compatibility bridge ignores the retained protected fields. > In [AbstractTask.java, lines 133–164](https://github.com/apache/dolphinscheduler/blob/872fc5095db90479cbfffac8839d844c1ce325fa/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractTask.java#L133-L164), getters and setters use only taskRequest, while needAlert and taskAlertInfo remain independently writable protected fields. An existing subclass that assigns these fields directly now gets false/null from the getters, and its alert data never reaches the success event. Please synchronize legacy field writes into the context before reporting task completion. Thanks for the review — both points are valid and have been fixed in commit 004a7c5f0. 1. Keep the log display limit separate from alert content Agreed. displayRows is only a log-display setting, so reusing it as the alert payload limit was a silent behavior change. The truncation has been removed from prepareTaskResultAlert: the alert now carries the complete query result (empty result sets still get a placeholder row, as before). The payload stays bounded by the query limit (QUERY_LIMIT, default 10000). 2. Compatibility bridge ignores the retained protected fields The @Deprecated getters/setters in AbstractTask now merge the legacy fields with the context: getNeedAlert() returns the field OR the context value, getTaskAlertInfo() prefers the field and falls back to the context, and both setters write through to both locations. On the executor side, PhysicalTaskExecutor.doTrackTaskPluginStatus() synchronizes these legacy values into the TaskExecutionContext once the task reaches SUCCEEDED — right before the success lifecycle event is built — so alert info written by third-party plugins via direct field assignment is no longer lost. Tests added/updated: AbstractTaskTest covers direct field assignment, setter write-through, and context fallback; SqlTaskTest now asserts the alert content keeps all rows regardless of displayRows. All tests pass (AbstractTaskTest 6/6, SqlTaskTest 27/27). -- 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]
