CAMEL-24761: camel-jbang - cache the Camel to Quarkus platform mapping - #27115
Brijesh-Thakkar wants to merge 4 commits into
Conversation
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The code is tidy and well tested, but the premise does not hold.
- The cached mapping never updates.
- The JIRA says that for a released Camel version the answer never changes. That is not the case:
findPlatformBompicks the newest release in each stream. - Example:
--camel-version 4.14.5resolves platform3.27.3.1and caches it. Quarkus then ships3.27.4, a security respin still on Camel 4.14.5. Before this PR, users picked it up within a day. With it, they stay on3.27.3.1until they pass--fresh. - This also contradicts
camel-4x-upgrade-guide-4_21.adoc(the newest compatible platform, fetched at most once a day). - Please give entries a TTL (e.g. a week), or cache only when the stream is no longer current. At minimum, document the pinning and
--fresh.
- The JIRA says that for a released Camel version the answer never changes. That is not the case:
- Docs. The caching and
--downloadbehaviour described in the 4.21 upgrade guide is now incomplete, and there is no camel-jbang doc update. - Minor.
storePlatformMappingdoes a non-atomic read-modify-write withFiles.writeString. With concurrent jbang processes, a partly written file is read as a miss and the other entries get dropped. Write to a temp file andATOMIC_MOVEinstead.
Claude Code on behalf of davsclaus
oscerd
left a comment
There was a problem hiding this comment.
Nicely guarded caching change, and the code matches the described contract.
The staleness risks are all handled:
- No SNAPSHOT caching —
snapshot = camelVersion.endsWith("-SNAPSHOT")and the store is gated on!snapshot, so a moving target is never remembered. - Only a final, exact resolution is stored — the write condition
resolved.isPresent() && camelVersion.equals(resolved.get().camelVersion())means a fallback (e.g.4.14.0→ a platform carrying4.14.5) or a not-yet-published release is not cached, so a later correct platform can still be discovered. This is the key correctness point and it's right. file://registries are excluded, and--freshremoves/bypasses the mapping.- Robust fallback — a missing/unreadable/corrupt mapping, or an entry with a blank
groupId/version, is treated as a cache miss (IOException | DeserializationExceptionon read), and write failures are swallowed, so the cache can never break normal resolution.
On a hit the QuarkusPlatformBom is rebuilt from the cached groupId/version plus the requested version and the caller's registry URI; because only exact matches are cached, the requested version equals the resolved Camel version, so the reconstruction is equivalent to the original. The public findQuarkusPlatformBom signature is unchanged (the registriesDir parameter is added only on an internal overload for the test), so run/export/catalog/doc/dependency/validate/MCP/Kubernetes callers are unaffected, and MCP/Kubernetes keep fresh=false.
LGTM. This is a fork PR so CI has not run yet; I'll hold the formal approval until the workflow is authorized and the checks are green.
This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd
|
Thanks @davsclaus findPlatformBom picks the newest
Will push an update shortly. |
…orm mapping cache
|
@davsclaus @oscerd pushed an update addressing the review:
Question on docs: CLAUDE.md says the upgrade guide is for migration notes, but |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after the follow-up commits addressing @davsclaus's three findings.
Finding 1 (TTL — cached mapping never updates): ✅ Addressed. Entries now carry a resolvedAt timestamp and expire after 7 days (PLATFORM_MAPPING_TTL). The readPlatformMapping guard rejects entries with age < 0 (future-dated) or age > TTL. The respinIsPickedUpAfterTtl test explicitly covers the scenario davsclaus described (platform 3.27.3.1 → 3.27.4 respin discovered after TTL expiry).
Finding 2 (Docs incomplete): ✅ Addressed. Both camel-4x-upgrade-guide-4_21.adoc and camel-jbang-devtools.adoc updated with the 7-day mapping cache description, SNAPSHOT exclusion, and --fresh behavior.
Finding 3 (Non-atomic write): ✅ Addressed. storePlatformMapping now uses Files.createTempFile in the same directory + Files.move(ATOMIC_MOVE) with AtomicMoveNotSupportedException fallback to REPLACE_EXISTING. Temp file cleanup in finally block. Concurrent last-writer-wins semantics documented in the Javadoc; a lost entry is only a cache miss.
No new issues. The Clock injection enables deterministic testing and the 15+ new tests cover all edge cases (TTL hit/miss, expired/future timestamps, corrupt mappings, unreadable paths, snapshot exclusion, respin after TTL, --fresh + --download=false interactions, and no temp file leaks).
CI has not run yet (fork PR); approval is contingent on CI passing.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the quick update, @Brijesh-Thakkar. The 7-day TTL with a resolvedAt stamp, the atomic temp-file write and the clock-driven tests (including the respin case) address the main concerns. One small doc fix remains: the new caching note was added to camel-4x-upgrade-guide-4_21.adoc, but this behaviour ships in 4.23 — please move it to the camel-jbang section of camel-4x-upgrade-guide-4_23.adoc, so 4.21 users are not told about a cache they do not have. The camel-jbang-devtools.adoc update looks good.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 6 of 697 tested, 8 compile-only — current: 6 all testedMaveniverse Scalpel detected 6 affected modules (current approach: 6). Skip-tests mode would test 6 modules (2 direct + 6 downstream), skip tests for 8 (generated code, meta-modules) Modules Scalpel would test (6)
Modules with tests skipped (8)
💡 Manual integration tests recommended:
All tested modules (16 modules, 3m 53s total)Total reactor time: 3m 53s
Top 20 slowest modules:
|
There was a problem hiding this comment.
This generally looks good to me (most of the pertinent comment have already been added).
I suggest adding a test on registry override isolation. The idea is to ensure that alternating the registry URI , whether via --quarkus-ext-registry CLI flag or the camel.jbang.quarkusExtensionRegistryBaseUri property, doesn't serve stale results from a different registry's cache.
We have been having issues in the past with the overrides not working as expected so it is better to test before.
…ade note to 4.23 The Quarkus Extension Registry cache directory was keyed on the host only, so two registries on the same host but different ports shared both the cached registry response and the platform mapping, and switching registries with --quarkus-ext-registry or camel.jbang.quarkusExtensionRegistryBaseUri could serve the other registry's platform. Include the port in the directory name; the default registry (no explicit port) keeps its directory. Move the caching note from the 4.21 upgrade guide to the camel-jbang section of the 4.23 guide, where the behaviour ships. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@davsclaus |
|
@gansheer |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 237123d addressing @davsclaus's doc location request and @gansheer's registry isolation request.
Finding 1 (doc note in 4_21 instead of 4_23): ✅ Addressed. The 3-line caching description removed from camel-4x-upgrade-guide-4_21.adoc and added to the camel-jbang section of camel-4x-upgrade-guide-4_23.adoc, now also covering the port-based directory isolation.
Finding 2 (registry override isolation — gansheer): ✅ Addressed. cacheFile() now keys the directory on host_port when the URI has an explicit port (uri.getPort() < 0 = default = host-only, backward compatible). Two new WireMock-based tests in QuarkusPlatformMixinTest (alternatingRegistryFlagDoesNotServeTheOtherRegistrysCache and alternatingRegistryPropertyDoesNotServeTheOtherRegistrysCache) confirm that switching registries on the same host with different ports returns each registry's own platform version and that each registry is queried exactly once.
Existing QuarkusHelperTest assertions updated from hardcoded "localhost" to registryDir() to account for the dynamic WireMock port — correct and thorough.
No new issues. All outstanding review items resolved.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after merge of main into the feature branch (237123d → 56e76b6). The 110 new commits are all from main — the PR's own files (QuarkusHelper.java, QuarkusHelperTest.java, QuarkusPlatformMixinTest.java, docs) are byte-identical to the previous review. All three reviewer findings (TTL, atomic write, registry isolation) remain addressed. CI passed. No issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, @Brijesh-Thakkar — the caching note is now in the camel-jbang section of the 4.23 upgrade guide and the 4.21 guide is untouched, so my earlier point is addressed. The per-port cache directory and the two WireMock tests for switching registries (flag and property) are a nice addition, and the default registry keeps its existing directory.
One small thing from the merge with main: the new paragraph is now directly followed by the camel validate source ... paragraph that landed on main at the same spot, with no blank line between them, so AsciiDoc renders the two unrelated notes as one paragraph. Please add an empty line after "...no longer share their cached responses." (suggestion inline). Otherwise LGTM.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| fetched at most once a day. Only a platform that carries exactly the requested Camel version is cached; a | ||
| `-SNAPSHOT` Camel version is never cached. `--fresh` clears both caches. A registry with an explicit port in | ||
| `--quarkus-ext-registry` (or `camel.jbang.quarkusExtensionRegistryBaseUri`) now has its own cache directory, so | ||
| two registries on the same host no longer share their cached responses. |
There was a problem hiding this comment.
After the merge with main this paragraph runs straight into the camel validate source ... paragraph below, so they render as one. Please add a blank line:
| two registries on the same host no longer share their cached responses. | |
| two registries on the same host no longer share their cached responses. | |
Description
CAMEL-24761: camel-jbang - cache the Camel version to Quarkus platform mapping
With
--runtime quarkus,QuarkusHelper.findQuarkusPlatformBomqueries the Quarkus extension registry (/client/platforms/all) to find the Quarkus platform matching the requested Camel version and then reads the platform BOM POMs through Maven.The registry response is already cached under
~/.camel/quarkus-extension-registries/, but the cache is only reused when it was written today (updatedToday). As a result, released Camel versions still trigger one registry request per day.This change persists the resolved Camel-to-Quarkus platform mapping alongside the existing registry cache and reuses it for 7 days when the mapping is exact and final.
Update after review
The first version of this PR cached the mapping without expiry, following the assumption in the JIRA that the mapping for a released Camel version never changes. @davsclaus pointed out that this is wrong:
findPlatformBompicks the newest release in each stream, so a respin on the same Camel version (e.g. Camel 4.14.5: platform3.27.3.1->3.27.4) would be missed until--freshwas used. This version fixes that:resolvedAttimestamp and expires after 7 days (PLATFORM_MAPPING_TTL).ATOMIC_MOVE.What changes
The caching logic is contained in
QuarkusHelper.findQuarkusPlatformBom. The public method signature is unchanged, so no caller changes are required forrun,export,catalog,doc,dependency,validate, MCP, or Kubernetes callers. The internal overload that already takesregistriesDir(used by tests) now also takes ajava.time.Clock; the public method usesClock.systemUTC().A new cache file is stored at:
~/.camel/quarkus-extension-registries/<host>/client/platforms/platform-mapping.jsonIt contains one entry per requested Camel version:
{ "4.14.5": { "groupId": "io.quarkus.platform", "version": "3.27.3.1", "resolvedAt": 1790000000000 } }resolvedAtis epoch milliseconds. The existingJsoner/JsonObjectAPI is used for reading and writing.On a cache hit, the same
QuarkusPlatformBomas the original resolution is reconstructed using the requested Camel version and the registry URI supplied by the caller.When the mapping is reused
A cached mapping is reused when:
file://registry-SNAPSHOT--freshis not setIn this case the registry, Maven, and the BOM POMs are not accessed.
An entry is a cache miss if
resolvedAtis missing, not a number, in the future, or older than the TTL, or ifgroupIdorversionis missing or blank. A miss falls through to the normal resolution, and a qualifying result overwrites the entry with a fresh timestamp. A platform respin on the same Camel version is therefore picked up within 7 days, or immediately with--fresh.When a mapping is stored
A mapping is stored only when the resolved platform's Camel version exactly matches the requested released Camel version.
For example, if Camel
4.14.0resolves to a platform containing Camel4.14.5, the result is not cached. Similarly, if a Camel release does not have a Quarkus platform published yet, the fallback result is not remembered. This ensures that temporary or non-exact resolutions do not prevent a later, correct platform from being discovered.Atomic write
storePlatformMappingwrites to a temp file created in the same directory (Files.createTempFile) and moves it over the target withATOMIC_MOVE, falling back toREPLACE_EXISTINGif the file system does not support atomic moves. The temp file is deleted in afinallyblock. A reader therefore never sees a partly written file. Concurrent jbang processes are last-writer-wins, and a lost entry is only a cache miss.--freshand--download=false--freshwith downloading removes the mapping file and queries the registry again.--download=falseuses a valid, non-expired mapping when available; otherwise it falls back to the existing registry cache behaviour.--fresh --download=falsecontinues to throw the existing "contradict each other" exception. This behaviour is unchanged.Failure handling
A missing, unreadable, or corrupt mapping file is treated as a cache miss and falls back to the existing resolution logic. Only
IOExceptionandDeserializationExceptionare handled when reading. Mapping write failures are ignored so they never affect the normal resolution flow.Not changed
QuarkusPlatformBomhas no such field and nothing reads it.fresh=falseare unchanged.file://registries are not cached.Clockis used only for the mapping.Tests
QuarkusHelperTestnow has 21 tests (18 added by this PR), all offline, using WireMock and per-test temporary directories. They do not use the real~/.cameldirectory or perform Maven downloads.The tests cover:
resolvedAt, after the first successful resolutionentryWithinTtlIsAHit)entryPastTtlIsRefreshed)3.27.3.1cached, 8 days later the registry returns3.27.4, and the result is3.27.4(respinIsPickedUpAfterTtl)resolvedAtor with a futureresolvedAtis a missSNAPSHOTversions are never cached or looked up--freshremoving and rebuilding the mapping--fresh --download=falsecontinuing to throw--download=falsewith and without an existing registry cacheresolvedAtfile://registries are not cachedThe tests use the
quarkus-registry-client-platforms.jsonfixture becauseregistry.quarkus.io/.../all.jsoncontains releases whose matching BOM POMs are not available in the test resources.Verification
Run with
./mvnw(Maven 3.9.16, as pinned by the repository)../mvnw formatter:format impsort:sortleaves the tree unchanged../mvnw -Psourcecheck validatepasses.QuarkusHelperTest: 21 tests, 0 failures.QuarkusPlatformMixinTest: passes.camel-jbang-coresuite: 1281 tests, 0 failures, 0 errors, 2 skipped.One test (
BindKnativeBrokerTest) failed once withConnection timed outand passed on rerun. The same intermittent failures occur onmainand are unrelated to this change.The exact command
mvn clean install -DskipTestswas not run. A-Dquicklyreactor install was used during development instead.Documentation
camel-jbang-devtools.adoc: describes the registry response cache (1 day), the resolved platform mapping cache (7 days, released versions with an exact match only), that SNAPSHOTs are never cached, and that--freshclears both.camel-4x-upgrade-guide-4_21.adoc: the caching note is updated to match.CLAUDE.mdsays the upgrade guide is for migration notes, so I am happy to drop this edit and keep only the devtools page if the maintainers prefer.Claude Code on behalf of Brijesh-Thakkar
Checklist
Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.