Copilot commented on code in PR #18210:
URL:
https://github.com/apache/dolphinscheduler/pull/18210#discussion_r3177494526
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-remoteshell/src/main/java/org/apache/dolphinscheduler/plugin/task/remoteshell/RemoteExecutor.java:
##########
@@ -235,24 +235,28 @@ public String runRemote(String command) throws
IOException {
private int runRemoteAndProcessLines(String command, Consumer<String>
lineConsumer) throws IOException {
try (
ChannelExec channel = getSession().createExecChannel(command);
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
ByteArrayOutputStream err = new ByteArrayOutputStream()) {
- InputStream out = channel.getInvertedOut();
+ channel.setOut(out);
channel.setErr(err);
channel.open();
+ channel.waitFor(EnumSet.of(ClientChannelEvent.CLOSED), 0);
+ Integer exitStatus = channel.getExitStatus();
+ if (exitStatus == null || exitStatus != 0) {
+ throw new TaskException(
+ "Remote shell task error, exitStatus: " + exitStatus +
" error message: " + err);
+ }
int readLines = 0;
- try (BufferedReader reader = new BufferedReader(new
InputStreamReader(out, StandardCharsets.UTF_8))) {
+ try (
+ BufferedReader reader = new BufferedReader(
+ new InputStreamReader(new
ByteArrayInputStream(out.toByteArray()),
+ StandardCharsets.UTF_8))) {
Review Comment:
`runRemoteAndProcessLines` buffers the entire stdout into a
`ByteArrayOutputStream`, then copies it again via `out.toByteArray()` before
line parsing. This negates the streaming/memory benefits introduced in #17800
and can double memory usage for large outputs. Consider processing lines from
the channel stream directly (or via a line-emitting OutputStream) to avoid
buffering+copying the whole output in memory.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-remoteshell/src/main/java/org/apache/dolphinscheduler/plugin/task/remoteshell/RemoteExecutor.java:
##########
@@ -235,24 +235,28 @@ public String runRemote(String command) throws
IOException {
private int runRemoteAndProcessLines(String command, Consumer<String>
lineConsumer) throws IOException {
try (
ChannelExec channel = getSession().createExecChannel(command);
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
ByteArrayOutputStream err = new ByteArrayOutputStream()) {
- InputStream out = channel.getInvertedOut();
+ channel.setOut(out);
channel.setErr(err);
channel.open();
+ channel.waitFor(EnumSet.of(ClientChannelEvent.CLOSED), 0);
+ Integer exitStatus = channel.getExitStatus();
+ if (exitStatus == null || exitStatus != 0) {
+ throw new TaskException(
+ "Remote shell task error, exitStatus: " + exitStatus +
" error message: " + err);
Review Comment:
The exception message concatenates `err` (a `ByteArrayOutputStream`)
directly, which uses the platform default charset in `toString()` and may
produce garbled text if the remote outputs UTF-8 (or another encoding). Convert
`err` to a String using a fixed charset (consistent with the UTF-8 reader)
before embedding it in the error message.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-remoteshell/src/main/java/org/apache/dolphinscheduler/plugin/task/remoteshell/RemoteExecutor.java:
##########
@@ -235,24 +235,28 @@ public String runRemote(String command) throws
IOException {
private int runRemoteAndProcessLines(String command, Consumer<String>
lineConsumer) throws IOException {
try (
ChannelExec channel = getSession().createExecChannel(command);
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
ByteArrayOutputStream err = new ByteArrayOutputStream()) {
- InputStream out = channel.getInvertedOut();
+ channel.setOut(out);
channel.setErr(err);
channel.open();
+ channel.waitFor(EnumSet.of(ClientChannelEvent.CLOSED), 0);
+ Integer exitStatus = channel.getExitStatus();
+ if (exitStatus == null || exitStatus != 0) {
+ throw new TaskException(
+ "Remote shell task error, exitStatus: " + exitStatus +
" error message: " + err);
+ }
int readLines = 0;
- try (BufferedReader reader = new BufferedReader(new
InputStreamReader(out, StandardCharsets.UTF_8))) {
+ try (
+ BufferedReader reader = new BufferedReader(
+ new InputStreamReader(new
ByteArrayInputStream(out.toByteArray()),
+ StandardCharsets.UTF_8))) {
String line;
while ((line = reader.readLine()) != null) {
readLines++;
lineConsumer.accept(line);
}
Review Comment:
`runRemoteAndProcessLines` throws on non-zero/null `exitStatus` before
consuming stdout via `lineConsumer`. This drops potentially useful stdout (and
prevents `track()` from logging/parsing whatever the command did emit) which
can make failures harder to diagnose. Consider consuming/forwarding stdout
lines first (or including stdout content in the exception) and only then
throwing based on `exitStatus`.
--
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]
