Conversation
…hile it runs, and ignores the compile work directory Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bp3538HRBPMQkb5ta9xRaj
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
reviewed
Sep 21, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
The core logic is correct — new directories are registered on-the-fly when they appear, and the compile work dir is excluded end-to-end. Three findings below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| if (workDir == null) { | ||
| return false; | ||
| } | ||
| Path work = Paths.get(workDir).toAbsolutePath().normalize(); |
Contributor
There was a problem hiding this comment.
isCompileWorkDir() is called on every file event (and on every directory ENTRY_CREATE event). Inside the method, getCamelContext().getCamelContextExtension().getContextPlugin(CompileStrategy.class) is resolved on each call, and Paths.get(workDir).toAbsolutePath().normalize() constructs a new Path object every time.
Since the compile work directory is immutable at runtime (set once before the watcher starts), this should be resolved once in doStart() and stored as a nullable field, e.g. private Path compileWorkDirPath. The check in isCompileWorkDir() then becomes a single startsWith with no allocation per event.
| if (workDir == null) { | ||
| return false; | ||
| } | ||
| Path work = Paths.get(workDir).toAbsolutePath().normalize(); |
Contributor
There was a problem hiding this comment.
🔧 Style: Paths.get() is the legacy NIO API (deprecated in spirit since Java 11). Use Path.of() instead — no extra import needed.
Suggested change
| Path work = Paths.get(workDir).toAbsolutePath().normalize(); | |
| Path work = Path.of(workDir).toAbsolutePath().normalize(); |
| Files.walkFileTree(dir, new SimpleFileVisitor<>() { | ||
| @Override | ||
| public FileVisitResult preVisitDirectory(Path d, BasicFileAttributes attrs) throws IOException { | ||
| if (isCompileWorkDir(d) || folderKeys.containsValue(d)) { |
Contributor
There was a problem hiding this comment.
🔍 O(n) lookup: folderKeys.containsValue(d) is a linear scan across all registered directories. For deep watched trees this runs once per node during registerNewDirectory's recursive walk. An inverse Set<Path> (or a second Map<Path, WatchKey>) would reduce this to O(1). Low severity since directory creation events are rare, but worth addressing if the watcher is used on large source trees.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 562 of 692 tested, 27 compile-only — current: 563 all testedMaveniverse Scalpel detected 590 affected modules (current approach: 563). Skip-tests mode would test 562 modules (3 direct + 560 downstream), skip tests for 27 (generated code, meta-modules)
|
| Module | Duration | Status |
|---|---|---|
| Camel :: Support | 15.0s | SUCCESS |
| Camel :: Docs | 11.4s | SUCCESS |
| Camel :: Core | n/a |
Top 20 slowest modules:
Camel :: Support(15.0s)Camel :: Docs(11.4s)
…d once at start, Path.of, and a set of the watched directories Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bp3538HRBPMQkb5ta9xRaj
gnodet-bot
approved these changes
Sep 21, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
All three findings from the previous review are addressed in 249c76d — Path.of instead of Paths.get, watchedFolders.contains (O(1) HashSet) instead of folderKeys.containsValue (O(n)), and resolveCompileWorkDir() called once at start rather than on every event.
The logic is correct end-to-end: registerNewDirectory walks the new tree, skips already-watched paths via watchedFolders, excludes the compile work dir via isCompileWorkDir, registers subdirectories, and feeds existing files into the changed list so they are not silently dropped. The ENTRY_CREATE-for-directory guard at line 366 prevents entering registerNewDirectory for a top-level compile work dir event. Thread safety is fine — all mutations to watchedFolders and folderKeys happen on the single WatchFileChangesTask thread.
Test is well-scoped: it covers the three scenarios that matter (new tree watched, subsequent files in it watched, class files in compile work dir not reloaded). The @DisabledIfSystemProperty exclusion on CI is the right call given watch service timing on virtual file systems.
Good fix.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
# Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc
gnodet-bot
approved these changes
Sep 21, 2026
gnodet-bot
approved these changes
Sep 21, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review after merge of main into the branch (merge commit 6bd5e7a).
The PR files are unchanged from the previous APPROVE at 249c76d — FileWatcherResourceReloadStrategy.java, the test, and the upgrade guide entry are byte-for-byte identical. The merge brings in three main commits (CAMEL-24860, CAMEL-24856, CAMEL-24854) and resolves a conflict in the upgrade guide by placing both entries in order. The conflict resolution is correct.
All findings from the initial review remain addressed. APPROVE stands.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The stepwise benchmark reported that a Java class added to a running
camel run --source-dir --devproject was not compiled on the reload. Reproducing it showed something narrower: a class added at the top of the source directory is compiled and usable within seconds; the class the benchmark added was undersrc/main/java/camel/example, and that tree was invisible to the watcher.FileWatcherResourceReloadStrategyregisters the directory tree once indoStartwhen recursive. The JDK watch service only reports events for registered directories, and a new directory arrives as one create event on its parent, which the watch task skipped withif (file.isDirectory()) continue. So every file written under a directory created after the start went unseen.src/main/java/com/acmeis created with its first file in it before the watcher can see the directory), and the files already in it are handled as changes. The macOS sensitivity modifier is kept for the new registrations.CompileStrategy.getWorkDir(),.camel-jbang/compilefor the CLI), at registration and in the event loop: the class files the runtime writes there when it compiles a source are not changes to watch, and with a*pattern they would trigger a reload, which compiles again, which writes again.Live check with
camel run --source-dir --devand the benchmark's sequence, class undersrc/main/java, a beans.yaml declaring it, the route using it: five new directories watched as they appeared, one reload for the class, the bean bound and the route logging "New order ORD-1001..." with no reload for the class file. Upgrade guide note added.Tests
FileWatcherResourceReloadStrategyTest(camel-core, real directories and a real watcher, local only since the watch service is too slow to time on the CI file systems): a tree created after the start with a file in it is reloaded, a file written later into the new tree is reloaded, a class file written into the compile work dir is not.🤖 Generated with Claude Code
https://claude.ai/code/session_01Bp3538HRBPMQkb5ta9xRaj