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.
Thanks for raising this. The size is already bounded at the statement level
before the result reaches the alert: executeQuery calls
stmt.setMaxRows(sqlParameters.getLimit() <= 0 ? QUERY_LIMIT :
sqlParameters.getLimit()) with QUERY_LIMIT = 10000, so the alert payload can
never contain more than the configured row limit — it is not an unbounded read.
The reason the alert currently carries the complete (bounded) result rather
than a smaller slice is that an earlier review asked us to keep the log-display
setting (displayRows) out of the alert payload and preserve the full query
result in the notification.
That said, I agree 10000 wide rows could still be a large payload. If you
prefer a tighter cap, I'm happy to introduce a separate, explicit alert limit
independent of displayRows — e.g. cap the alert content at N rows with a
truncation marker. Would that work for you, and what limit would you suggest?
--
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]
