wank125 opened a new pull request, #18719:
URL: https://github.com/apache/dolphinscheduler/pull/18719
## Was this PR generated or assisted by AI?
YES. The code change, tests and this description were drafted with an AI
coding agent under my direction, and I reviewed every line.
## Purpose of the pull request
Close #18714 (scoped to the log-timing part, per the maintainer feedback in
the issue).
In `SqlTask#resultProcess`, the `displayRows` rows are currently logged only
after the whole result set has been buffered into `resultJSONArray`, in a
second traversal over the array. For a long-running query with a large result
set, no row output appears in the worker log until the entire result has been
fetched.
As discussed in the issue, this PR deliberately keeps the change to the
log-timing improvement only: log rows inline as each row is read and drop the
second traversal. The `limitAlertContent` serialization optimization is
intentionally NOT included, to keep the implementation simple.
## Brief change log
- `dolphinscheduler-task-sql`: `SqlTask#resultProcess` now logs each of the
first `displayRows` rows inline while iterating the `ResultSet`. The header
message now states the configured cap ("up to N rows") because the total row
count is unknown before the result set is fully read.
- The task return value and alert payload behavior is unchanged: the full
result set is still buffered and returned, and the per-row log format ("row N :
{...}") is unchanged.
- `dolphinscheduler-task-sql` pom: exclude `slf4j-reload4j` (transitively
pulled via `hadoop-client`) and declare `logback-classic` in test scope, so the
module has a single deterministic SLF4J binding for the new log assertions.
## Verify this pull request
This change added tests and can be verified as follows:
- Added
`SqlTaskTest#testResultProcess_LogsDisplayRowsWhileReadingResultSet`: captures
`SqlTask` logs with a Logback `ListAppender` and asserts all rows are already
logged at the moment the `ResultSet` is exhausted (i.e. logging is inline while
reading). This test fails on the previous implementation (0 rows logged at
exhaustion).
- Added `SqlTaskTest#testResultProcess_DisplayRowsLimit_LogsOnlyFirstRows`:
with `displayRows=2` and a 5-row result set, only rows 1-2 are logged while the
returned result still contains all 5 rows.
- Manually verified the change by running `mvn -pl
dolphinscheduler-task-plugin/dolphinscheduler-task-sql -am test` locally (JDK
8): all 21 tests of the module pass, and `spotless:check` passes.
--
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]
