gnodet-bot commented on code in PR #27516:
URL: https://github.com/apache/camel/pull/27516#discussion_r4212471647
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/LaunchManager.java:
##########
@@ -358,38 +418,70 @@ private void checkDeferredLaunch(long now) {
DeferredLaunch dl = deferredLaunch;
deferredLaunch = null;
dl.launchAction.run();
- } else if (now - deferredLaunch.startTime > 120_000) {
+ } else if (!infraStarting(deferredLaunch.requiredInfra)) {
+ // the infra failed to start: the failure log says why
+ DeferredLaunch dl = deferredLaunch;
+ deferredLaunch = null;
+ notify("Not started: " + dl.displayName() + " (its infra
services failed to start)", true);
+ } else if (now - deferredLaunch.startTime > INFRA_WATCH_MS) {
+ // a first start pulls the container image, which can take
minutes: wait while the infra is starting
deferredLaunch = null;
notify("Timeout waiting for infra services to start", true);
}
}
}
private void monitorPendingLaunches(long now) {
+ Set<String> runningAliases = null;
Iterator<PendingLaunch> it = pendingLaunches.iterator();
while (it.hasNext()) {
PendingLaunch pl = it.next();
+ if (pl.infraAlias != null && !pl.started) {
+ if (runningAliases == null) {
+ runningAliases = infraServices.get().stream()
+ .filter(i -> i.alive).map(i ->
i.alias).collect(Collectors.toSet());
+ }
+ // an infra service has started when it is up
+ if (runningAliases.contains(pl.infraAlias)) {
+ pl.started = true;
+ notify("Started: " + pl.name, false);
+ pl.announced = true;
+ }
+ }
if (!pl.process.isAlive()) {
int exitCode = pl.process.exitValue();
- if (exitCode == 0 || pl.started) {
+ boolean ok = pl.infraAlias != null ? pl.started : exitCode ==
0 || pl.started;
+ if (ok) {
if (!pl.announced) {
notify("Started: " + pl.name, false);
}
- } else if (failureLogCallback != null) {
- failureLogCallback.accept(pl.name, pl.outputFile);
+ outcomes.put(pl.name, LaunchOutcome.started());
+ } else {
+ outcomes.put(pl.name, LaunchOutcome.failed(pl.outputFile));
+ if (failureLogCallback != null) {
+ failureLogCallback.accept(pl.name, pl.outputFile);
+ }
}
it.remove();
} else if (pl.started) {
// up and running: a stop or a failure from now on is not a
failed start
+ outcomes.put(pl.name, LaunchOutcome.started());
it.remove();
} else if (pl.startFailed()) {
// the app gave up starting (port in use, build failure) but
its JVM lives on: stop it, and show why
pl.process.descendants().forEach(ProcessHandle::destroy);
pl.process.destroy();
+ outcomes.put(pl.name, LaunchOutcome.failed(pl.outputFile));
if (failureLogCallback != null) {
failureLogCallback.accept(pl.name, pl.outputFile);
}
it.remove();
+ } else if (pl.infraAlias != null) {
+ if (now - pl.startTime > INFRA_WATCH_MS) {
+ // not up after the longest an image pull should take: a
failed start, as the other ends record
+ outcomes.put(pl.name, LaunchOutcome.failed(pl.outputFile));
+ it.remove();
Review Comment:
⚠️ **Missing `failureLogCallback` on infra timeout path.**
Both the process-exit failure path (line ~460) and the `startFailed()` path
(line ~475) call `failureLogCallback` when a launch fails, so the TUI can show
the output. This infra-timeout branch records `LaunchOutcome.failed(...)` but
never calls the callback — the failure will be silent from the user's
perspective (no failure log popup).
```suggestion
outcomes.put(pl.name,
LaunchOutcome.failed(pl.outputFile));
if (failureLogCallback != null) {
failureLogCallback.accept(pl.name, pl.outputFile);
}
it.remove();
```
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/LaunchManager.java:
##########
@@ -414,6 +506,49 @@ private void notify(String msg, boolean error) {
*/
static final long WATCH_MS = 5 * 60_000;
+ // how the latest launch of each name went, for an agent that asks
(tui_run_example waits for it)
+ private final Map<String, LaunchOutcome> outcomes = new
ConcurrentHashMap<>();
+
+ /**
+ * How a launch went: started (Camel said so), or failed with the end of
its output; null while it is starting.
+ */
+ record LaunchOutcome(boolean ok, String log) {
+
+ private static final Pattern ANSI =
Pattern.compile("\u001B\\[[0-9;]*m");
+
+ static LaunchOutcome started() {
+ return new LaunchOutcome(true, null);
+ }
+
+ static LaunchOutcome failed(Path outputFile) {
+ String tail = null;
+ try {
+ // without colors and stack frames: the messages and their
causes are what tell why
+ List<String> lines = Files.readAllLines(outputFile,
StandardCharsets.UTF_8).stream()
+ .map(l -> ANSI.matcher(l).replaceAll(""))
+ .filter(l -> !l.stripLeading().startsWith("at ") &&
!l.stripLeading().startsWith("... "))
+ .toList();
Review Comment:
💡 **Minor: `Files.readAllLines` materialises the full log before filtering.**
`readAllLines` loads the entire file into a `List<String>`. For infra
services whose container image pull takes several minutes, the log can be
large. Using `Files.lines(outputFile, UTF_8)` returns a lazy stream and avoids
loading lines that are then discarded. The last-30 tail selection can still be
done in a single pass with a fixed-size ring buffer or `Collectors.toList()` +
`subList`.
Low severity — log files are typically small — but worth noting for very
verbose pulls.
--
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]
