njnu-seafish commented on code in PR #18549:
URL:
https://github.com/apache/dolphinscheduler/pull/18549#discussion_r4237199032
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sql/src/main/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTask.java:
##########
@@ -287,13 +288,18 @@ private String resultProcess(ResultSet resultSet) throws
Exception {
}
}
- String result = resultJSONArray.isEmpty() ?
JSONUtils.toJsonString(generateEmptyRow(resultSet))
- : JSONUtils.toJsonString(resultJSONArray);
+ String result;
+ ArrayNode alertArray = resultJSONArray;
+ if (resultJSONArray.isEmpty()) {
+ ArrayNode emptyRow = generateEmptyRow(resultSet);
+ result = JSONUtils.toJsonString(emptyRow);
+ alertArray = emptyRow;
+ } else {
+ result = JSONUtils.toJsonString(resultJSONArray);
+ }
Review Comment:
> Need to control the size; otherwise, an OOM error is likely to occur here.
Regarding the OOM concern: displayRows only governs log output — the alert
content still carries the full result set (see
testPrepareTaskResultAlertKeepsFullContentRegardlessOfDisplayRows), which is
the direction SbloodyS settled on ("preserve the full alert result").
On the 10000-row bound: setMaxRows uses QUERY_LIMIT=10000 only as a default
when no limit is configured; it's user-overridable with no hard cap, so it's a
soft default rather than a guaranteed ceiling. An alert-side limit wouldn't
meaningfully help either — the result rows are materialized into the ArrayNode
before alert serialization, so trimming at the alert stage happens after the
memory pressure point. The effective lever is capping setMaxRows at the source.
I'd prefer to keep this PR to the dedup / alert-content scope and follow up
on the setMaxRows hardening in a separate issue if you agree — or let me know
if you'd rather address it here.
--
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]
