gnodet-bot commented on code in PR #28108:
URL: https://github.com/apache/camel/pull/28108#discussion_r4313173737
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -2119,6 +2146,85 @@ private static void applyVersion(ToolContext ctx,
Map<String, String> args) {
*
* @throws ToolExecutionException when no process can be selected
*/
Review Comment:
🐛 **Orphaned javadoc:** the `*/` on this line closes the `selectProcess()`
javadoc that was just before (which belonged to `selectProcess()` ~80 lines
further down). The new `previousLog()` javadoc opens immediately after on the
next line, so the file now has two consecutive `/** … */` blocks with nothing
between them — the first one is dangling. The `selectProcess()` javadoc that
ends here should be moved back to sit directly above `selectProcess()` itself.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -2119,6 +2146,85 @@ private static void applyVersion(ToolContext ctx,
Map<String, String> args) {
*
* @throws ToolExecutionException when no process can be selected
*/
+ /**
+ * The log of the previous run of the integration (CAMEL-25784): the last
run of it that ended, such as one that
+ * failed to start, found by the name or pid given, else by the pid
selected before. Its message says how that run
+ * ended.
+ *
+ * @param notRunning why the integration was not found
running, or null when the previous run was asked
+ * for
+ * @throws ToolExecutionException when no previous run matches, or more
than one (notRunning, when given)
+ */
+ private static JsonObject previousLog(ToolContext ctx, Map<String, String>
args, ToolExecutionException notRunning) {
+ PreviousRun.Info info = previousRun(ctx, args, notRunning);
+ JsonObject answer = new JsonObject();
+ answer.put("message", (notRunning != null ? info.name() + " is not
running. " : "")
+ + "This is the log of its previous run" +
(info.pid() > 0 ? " (pid " + info.pid() + ")" : "")
+ + ", which " + info.howItEnded());
+ JsonObject previous = new JsonObject();
+ previous.put("name", info.name());
+ if (info.pid() > 0) {
+ previous.put("pid", info.pid());
+ }
+ previous.put("ended", info.endedAt());
+ if (info.exitCode() != null) {
+ previous.put("exitCode", info.exitCode());
+ }
+ if (info.failure() != null) {
+ previous.put("failure", info.failure());
+ }
+ answer.put("previous", previous);
+ answer.putAll(LogFileReader.read(info.log(), integer(args, "limit",
50), args.get("filter"), args.get("level"),
+ bool(args, "details", false)));
+ return answer;
+ }
+
+ /**
+ * The error of an integration that is not running, with how its previous
run ended when there is one.
+ */
+ private static ToolExecutionException notRunning(
+ ToolContext ctx, Map<String, String> args, ToolExecutionException
notRunning) {
+ PreviousRun.Info info;
+ try {
+ info = previousRun(ctx, args, notRunning);
+ } catch (ToolExecutionException e) {
+ return notRunning;
+ }
+ return new ToolExecutionException(
+ notRunning.getMessage() + ". Its previous run"
+ + (info.pid() > 0 ? " (pid " +
info.pid() + ")" : "") + " " + info.howItEnded()
+ + ". camel_get_log shows its log.");
+ }
+
+ // the one previous run of the name or pid given, else of the pid selected
before
+ private static PreviousRun.Info previousRun(
+ ToolContext ctx, Map<String, String> args, ToolExecutionException
notRunning) {
+ String name = args.get("name");
+ String key = name != null && !name.isBlank() ? name : ctx.pid() >= 0 ?
Long.toString(ctx.pid()) : null;
Review Comment:
💡 **Nested ternary:** the chained ternary is hard to parse at a glance.
Adding parentheses around the inner expression makes the precedence explicit
and helps readers:
```suggestion
String key = name != null && !name.isBlank() ? name : (ctx.pid() >=
0 ? Long.toString(ctx.pid()) : null);
```
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/Run.java:
##########
@@ -3052,26 +3054,63 @@ protected int runCustomCamelVersion(KameletMain main)
throws Exception {
protected int runKameletMain(KameletMain main) throws Exception {
Recording jfrRecording = startJfrRecording();
+ PreviousRun.Keeper previousLog = null;
+ if (logFile != null && keepsPreviousLog()) {
+ // CAMEL-25784: the log is kept as the previous log of the
integration when the run ends, and so are the
+ // logs that runs which were killed left behind
+ PreviousRun.keepOrphans();
+ previousLog = new PreviousRun.Keeper(logFile, () -> runName(main));
+ }
+ Integer exitCode = null;
+ String failure = null;
try {
main.start();
if (hasNoRoutes(main)) {
// it would run doing nothing: a file without routes, or a
mistake in it
- printer().printErr(files.isEmpty()
- ? "No routes found" : "No routes found in: " +
String.join(", ", files));
+ failure = files.isEmpty() ? "No routes found" : "No routes
found in: " + String.join(", ", files);
+ printer().printErr(failure);
main.stop();
- return CliExitCode.FAILURE;
+ exitCode = CliExitCode.FAILURE;
+ return exitCode;
}
main.run();
+ int code = main.getExitCode();
+ // no exit code is set when Camel is stopped from outside (such as
camel stop)
+ exitCode = code == Integer.MIN_VALUE ? CliExitCode.OK : code;
+ return exitCode;
+ } catch (Exception | Error e) {
Review Comment:
❓ **`catch (Exception | Error e)`:** catching `Error` is unusual — the
comment only explains the `Exception` case ("such as a route that failed to
start"). `Error` subtypes like `OutOfMemoryError` or `StackOverflowError` are
normally not meant to be caught and handled. Is this intentional? If there is a
specific `Error` subtype you need to capture for the failure log (e.g.
`AssertionError` in tests?), it would be clearer to list it explicitly or
update the comment to explain why `Error` is included.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/PreviousRun.java:
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dsl.jbang.core.common;
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.RandomAccessFile;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.time.Instant;
+import java.time.ZoneId;
+import java.time.format.DateTimeFormatter;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.apache.camel.support.PatternHelper;
+import org.apache.camel.util.TimeUtils;
+import org.apache.camel.util.json.JsonObject;
+import org.apache.camel.util.json.Jsoner;
+
+/**
+ * The log of the previous run of an integration, as <tt>kubectl logs
--previous</tt> shows the log of the previous
+ * instance of a container in Kubernetes.
+ * <p/>
+ * When <tt>camel run</tt> ends (it was stopped, it failed to start, or it
stopped by itself), its log is kept as
+ * <tt>~/.camel/<name>-previous.log</tt>, by the name of the
integration, with
+ * <tt>~/.camel/<name>-previous.json</tt>, which says when and how the
run ended. The next run of that name that
+ * ends replaces both. Like Kubernetes, at most the last 10 MB of a log are
kept.
+ * <p/>
+ * A process that is gone without keeping its log (it was killed, or its JVM
crashed) leaves its
+ * <tt>~/.camel/<pid>.log</tt> behind. That log is kept for it, under
the name in its status file, the next time
+ * the previous runs are looked up, and the next time an integration is run.
+ */
+public final class PreviousRun {
+
+ /**
+ * The end of a log that is kept, at most: Kubernetes keeps at most 10 MB
of the log of a container by default.
+ */
+ static final long MAX_SIZE = 10L * 1024 * 1024;
+
+ /**
+ * How old the log of a process that is gone may get, when there is no
name to keep it by, before it is removed.
+ */
+ static final long ORPHAN_AGE = TimeUnit.DAYS.toMillis(1);
+
+ /**
+ * What a process that is gone without keeping its log ended with, as far
as is known.
+ */
+ static final String GONE = "the process was gone without stopping Camel
(killed, or its JVM crashed)";
+
+ private static final String LOG_SUFFIX = "-previous.log";
+ private static final String INFO_SUFFIX = "-previous.json";
+ private static final Pattern PID_LOG = Pattern.compile("(\\d+)\\.log");
+ private static final int MAX_FAILURE = 500;
+ private static final DateTimeFormatter TIME
+ = DateTimeFormatter.ofPattern("yyyy-MM-dd
HH:mm:ss").withZone(ZoneId.systemDefault());
+
+ /**
+ * A run that ended, and the log it kept.
+ *
+ * @param name the name of the integration
+ * @param pid the pid it ran as, 0 when not known
+ * @param directory the directory it ran in, or <tt>null</tt>
+ * @param started when it started, in epoch millis, 0 when not known
+ * @param ended when it ended, in epoch millis
+ * @param exitCode the exit code, or <tt>null</tt> when not known (such
as of a process that was killed)
+ * @param failure why it ended, when it failed (such as the error it
failed to start with), or <tt>null</tt>
+ * @param log the log it kept
+ */
+ public record Info(String name, long pid, String directory, long started,
long ended, Integer exitCode,
+ String failure, Path log) {
+
+ /**
+ * Which run it was, and how it ended, in one line.
+ */
+ public String describe() {
+ return "The previous run of " + name + (pid > 0 ? " (pid " + pid +
")" : "") + " " + howItEnded();
+ }
+
+ /**
+ * When the run ended, in local time, such as <tt>2026-10-19
10:32:01</tt>.
+ */
+ public String endedAt() {
+ return TIME.format(Instant.ofEpochMilli(ended));
+ }
+
+ /**
+ * How the run ended: when, with which exit code, and why when it
failed.
+ */
+ public String howItEnded() {
+ StringBuilder sb = new StringBuilder("ended at
").append(endedAt());
+ long ago = System.currentTimeMillis() - ended;
+ if (ago >= 0) {
+ sb.append(" (").append(TimeUtils.printDuration(ago,
false)).append(" ago)");
+ }
+ if (exitCode != null) {
+ sb.append(" with exit code ").append(exitCode);
+ }
+ if (failure != null) {
+ sb.append(": ").append(failure);
+ }
+ return sb.toString();
+ }
+ }
+
+ private PreviousRun() {
+ }
+
+ /**
+ * Keeps the log of a run of this JVM that ended as the previous log of
its name.
+ *
+ * @param log the log of the run
+ * @param name the name of the integration
+ * @param pid the pid it ran as, 0 when not known
+ * @param exitCode the exit code, or <tt>null</tt> when not known
+ * @param failure why it ended, when it failed, or <tt>null</tt>
+ * @return the kept log, or <tt>null</tt> when there was no log
to keep
+ */
+ public static Path keep(Path log, String name, long pid, Integer exitCode,
String failure) {
+ long started =
ProcessHandle.current().info().startInstant().map(Instant::toEpochMilli).orElse(0L);
+ return keep(log, new Info(
+ name, pid, System.getProperty("user.dir"), started,
System.currentTimeMillis(),
+ exitCode, failure, null));
+ }
+
+ /**
+ * Keeps the log of a run that ended as the previous log of its name, in
place of the one kept before.
+ *
+ * @return the kept log, or <tt>null</tt> when there was no log to keep
+ */
+ static Path keep(Path log, Info info) {
+ if (log == null || info.name() == null || info.name().isBlank() ||
!Files.isRegularFile(log)) {
+ return null;
+ }
+ Path dir = CommandLineHelper.getCamelDir();
+ String file = fileName(info.name());
+ Path target = dir.resolve(file + LOG_SUFFIX);
+ try {
+ if (Files.size(log) > MAX_SIZE || !move(log, target)) {
+ copyEnd(log, target, MAX_SIZE);
+ try {
+ Files.deleteIfExists(log);
+ } catch (IOException e) {
+ // still open (Windows): removed when the process is gone
(see keepOrphans)
+ }
+ }
+ JsonObject jo = new JsonObject();
+ jo.put("name", info.name());
+ if (info.pid() > 0) {
+ jo.put("pid", info.pid());
+ }
+ if (info.directory() != null) {
+ jo.put("directory", info.directory());
+ }
+ if (info.started() > 0) {
+ jo.put("started", info.started());
+ }
+ jo.put("ended", info.ended());
+ if (info.exitCode() != null) {
+ jo.put("exitCode", info.exitCode());
+ }
+ if (info.failure() != null) {
+ String failure = info.failure().strip();
+ jo.put("failure", failure.length() > MAX_FAILURE ?
failure.substring(0, MAX_FAILURE) + "..." : failure);
+ }
+ Files.writeString(dir.resolve(file + INFO_SUFFIX), jo.toJson());
Review Comment:
💡 **Non-atomic log/info pair:** the log file is moved/copied first (lines
162–164), then the JSON metadata file is written last (this line). If the JVM
crashes between the two operations the log has been updated but the `.info`
file still describes the previous run, making the pair inconsistent. Writing
the JSON file *before* moving the log would be safer: a reader that finds no
log but a stale info can treat it as incomplete, whereas a moved log with no
info is ambiguous.
--
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]
