gnodet-bot commented on code in PR #27909:
URL: https://github.com/apache/camel/pull/27909#discussion_r4285637629
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/action/CamelMemoryLeak.java:
##########
@@ -139,38 +139,38 @@ private int doDualRecording(long pid) throws Exception {
printer().printf("Recording 1 of 2 (%ds)...%n", dur1);
JsonObject startResp = sendAction(pid, "start", dur1);
if (startResp == null ||
!"recording".equals(startResp.getString("status"))) {
- printer().println("Error: Failed to start recording 1");
+ printer().printErr("Failed to start recording 1");
return 1;
Review Comment:
**Incomplete exit code migration** — `doDualRecording` still returns
hardcoded `1` instead of `CliExitCode.FAILURE` after migrating the error
messages in this PR. Same on lines 151, 159, 167, 174, and 227 in
`handleResponse`. The `doCall()` method (not in the diff) also returns `1` when
no PID matches without printing any error at all — a script gets exit code 1
with no diagnostic output. Since this file is already in the PR, these should
be fixed here.
```suggestion
return CliExitCode.FAILURE;
```
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/action/CamelMemoryLeak.java:
##########
@@ -139,38 +139,38 @@ private int doDualRecording(long pid) throws Exception {
printer().printf("Recording 1 of 2 (%ds)...%n", dur1);
JsonObject startResp = sendAction(pid, "start", dur1);
if (startResp == null ||
!"recording".equals(startResp.getString("status"))) {
- printer().println("Error: Failed to start recording 1");
+ printer().printErr("Failed to start recording 1");
return 1;
}
waitForRecordingComplete(pid, dur1);
JsonObject stopResp = sendAction(pid, "stop", 0);
if (stopResp == null ||
!"completed".equals(stopResp.getString("status"))) {
- printer().println("Error: Recording 1 did not complete");
+ printer().printErr("Recording 1 did not complete");
return 1;
}
// run 2
printer().printf("Recording 2 of 2 (%ds)...%n", dur2);
startResp = sendAction(pid, "start", dur2);
if (startResp == null ||
!"recording".equals(startResp.getString("status"))) {
- printer().println("Error: Failed to start recording 2");
+ printer().printErr("Failed to start recording 2");
return 1;
}
waitForRecordingComplete(pid, dur2);
stopResp = sendAction(pid, "stop", 0);
if (stopResp == null ||
!"completed".equals(stopResp.getString("status"))) {
- printer().println("Error: Recording 2 did not complete");
+ printer().printErr("Recording 2 did not complete");
return 1;
}
// compare
JsonObject cmpResp = sendAction(pid, "compare", 0);
if (cmpResp == null) {
- printer().println("Error: Failed to get comparison data");
+ printer().printErr("Failed to get comparison data");
return 1;
Review Comment:
**Timeout case returns `FAILURE (1)`, not `TIMEOUT (124)`.** `cmpResp ==
null` means the integration did not respond in time — that is a timeout, not a
generic failure. In `doCall()` (not in the diff), `jo == null` at line 128 has
the same issue with `printer().println()` on stdout and `return 1`. The PR
description explicitly states timeout should be 124 (as done for `cmd sql`). A
script checking `$? == 124` to retry will loop forever.
```suggestion
return CliExitCode.TIMEOUT;
```
--
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]
