gnodet-bot commented on code in PR #28150:
URL: https://github.com/apache/camel/pull/28150#discussion_r4323758527
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -1845,9 +1853,24 @@ public static JsonObject listFiles(Path dir) {
result.put("configFiles", configFiles);
result.put("files", files);
result.put("totalFiles", all.size());
+ // a file consumer moves a file it read into .camel (without
noop=true), which the listing skips
+ Map<String, Integer> consumed = consumedFolders(dir);
+ String moved = null;
+ if (!consumed.isEmpty()) {
+ JsonObject folders = new JsonObject();
+ folders.putAll(consumed);
+ result.put("consumedFiles", folders);
+ moved = String.join(", ", consumed.entrySet().stream().map(e ->
e.getKey() + " holds " + e.getValue()
+ +
(e.getValue() == 1 ? " file" : " files"))
+ .toList())
+ + " that a file consumer moved there after reading
(noop=true leaves the files where they are)";
+ }
if (all.isEmpty()) {
- result.put("message", "The directory has no files");
- } else if (all.size() > MAX_FILES) {
+ result.put("message", moved != null ? "The directory has no files,
but " + moved : "The directory has no files");
+ } else if (moved != null) {
+ result.put("note", moved);
+ }
+ if (all.size() > MAX_FILES) {
Review Comment:
💡 **`all.isEmpty()` / `MAX_FILES` restructuring:** The previous code used
`else if` for the `MAX_FILES` check, making it explicit that the branch is
unreachable when `all` is empty. The new code splits it into a separate `if`,
which is logically equivalent (`all.isEmpty()` → `all.size() == 0` → never `>
MAX_FILES`) but slightly less obvious. A brief comment `// all is non-empty
here` or keeping `else if` would make the invariant clear.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -2271,6 +2294,91 @@ static int integer(Map<String, String> args, String key,
int defaultValue) {
}
}
+ /**
+ * Marks the failed exchanges of an earlier version of the routes: those
that failed before the newest reload of the
+ * routes in the log. The errors are kept until the integration restarts,
and an agent that read them after a change
+ * took them for errors of its new version, and changed routes that
already worked.
+ */
+ static JsonObject markBeforeLastReload(JsonObject errors, long pid, String
name) {
+ try {
+ JsonObject log = LogFileReader.read(pid, name, 1,
ReloadOutcome.RELOADED, null);
+ if (log.get("lines") instanceof List<?> lines && !lines.isEmpty()
&& lines.get(0) instanceof JsonObject reload) {
+ return markBefore(errors, reload.getString("time"),
LocalDateTime.now());
+ }
+ } catch (Exception e) {
+ // no log to tell: the errors as they are
+ }
+ return errors;
+ }
+
+ private static final long RELOAD_MARGIN_MILLIS = 1000;
+
+ /** Marks the errors that failed before the reload at the given time of
day; the log keeps the time of day only. */
+ static JsonObject markBefore(JsonObject errors, String reloadTime,
LocalDateTime now) {
+ LocalTime time;
+ try {
+ time = LocalTime.parse(reloadTime);
+ } catch (Exception e) {
+ return errors;
+ }
+ LocalDateTime reload = now.toLocalDate().atTime(time);
+ if (reload.isAfter(now)) {
+ reload = reload.minusDays(1);
+ }
+ // the summary is logged after the new routes started: a margin keeps
a route that fails at once unmarked
+ long reloadMillis =
reload.atZone(ZoneId.systemDefault()).toInstant().toEpochMilli() -
RELOAD_MARGIN_MILLIS;
Review Comment:
💡 **DST edge case in `markBefore`:**
`LocalDateTime.atZone(ZoneId.systemDefault())` silently adjusts the time during
a spring-forward gap (e.g. the clock jumps from 02:00 to 03:00 and a reload
time of 02:30 is shifted to 03:30). The 1-second `RELOAD_MARGIN_MILLIS` does
not cover that 1-hour shift, so errors that occurred in the gap could be
incorrectly marked as `beforeLastReload`.
If the system never crosses DST boundaries or the use-case tolerates ±1h
margin this is fine, but a Javadoc note acknowledging the limitation would help
future maintainers.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/HeaderChecks.java:
##########
@@ -139,12 +164,64 @@ static List<String> timerCounterComparisons(String
content) {
return msgs;
}
+ /**
+ * The {@link #WRONG_NAMES} the content uses as header names, and the
names written as a constant that Exchange does
+ * not have, which fail the route when Camel creates it; once each, the
names go into reported.
+ */
+ static List<String> wrongNames(String content, Set<String> reported) {
+ List<String> msgs = new ArrayList<>();
+ String[] lines = content.split("\n", -1);
+ for (int i = 0; i < lines.length; i++) {
+ Matcher c = EXCHANGE_CONSTANT_NAME.matcher(lines[i]);
+ if (c.find() && isSetOrRemoveName(lines, i)) {
+ String hint = exchangeConstantHint(c.group(1));
+ if (hint != null && reported.add("Exchange." + c.group(1))) {
+ msgs.add("Line " + (i + 1) + ": " + hint);
+ }
+ }
+ Matcher m = CAMEL_HEADER_REF_PATTERN.matcher(lines[i]);
+ while (m.find()) {
Review Comment:
💡 **`find()` vs `while (find())` asymmetry:** `EXCHANGE_CONSTANT_NAME` uses
`if (c.find())` (first match only) while `CAMEL_HEADER_REF_PATTERN` uses `while
(m.find())` (all matches per line). Since `EXCHANGE_CONSTANT_NAME` anchors on
`name:\s*`, which appears at most once per YAML line, single-match is safe here
— but the asymmetry warrants a brief comment to make the intent explicit.
--
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]
