CAMEL-23239: Add camel-state-store component with pluggable key-value store - #22158
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
a434f02 to
1e1a769
Compare
|
Hm, this issue doesn't seem to exist on ASF Jira. |
|
The title refers to the wrong issue, CAMEL-23228 is "Add DataWeave to DataSonnet transpiler in camel-jbang". |
|
the commit message also needs to be updated with the correct jira issue number |
apupier
left a comment
There was a problem hiding this comment.
Can you elaborate on the difference between these components and the existing Camel caffeine cache component? https://camel.apache.org/components/4.18.x/caffeine-cache-component.html
1e1a769 to
e3ba53c
Compare
|
Thanks for the reviews! I've pushed an update:
@apupier — regarding the difference with Claude Code on behalf of Guillaume Nodet |
Design note:
|
9acd8bf to
8bbae8d
Compare
ba0b5d2 to
79835f9
Compare
|
@gnodet this needs to be changed tro 4.23.0-SNAPSHOT |
79835f9 to
02a5411
Compare
02a5411 to
5c93f9b
Compare
davsclaus
left a comment
There was a problem hiding this comment.
Review of camel-state-store
Thanks for this work, @gnodet — the component is well-structured, follows Camel conventions well, and has solid test coverage across all four backends. Here are the findings from the review.
Critical
- MojoHelper missing backend module registrations —
MojoHelper.getComponentPath()only registerscamel-state-storebut notcamel-state-store-caffeine,camel-state-store-redis,camel-state-store-infinispan. Without this, catalog/docs generation will not discover the backend modules' metadata. The fix should useArrays.asList(...)to include all four sub-modules, similar to howcamel-testregisters its sub-modules.
Medium
-
StateStoreComponent.doStop()resource leak — If one backend'sstop()throws, subsequent backends are never stopped. Eachstop()call should be wrapped in try-catch so all backends get a chance to clean up. -
Case-sensitive operation lookup —
StateStoreProducer.determineOperation()usesStateStoreOperations.valueOf(headerOp.toString())which throws a crypticIllegalArgumentExceptionfor"PUT"vs"put". Consider case-insensitive lookup or a try-catch with a user-friendly error message listing valid operations. -
Infinispan
Thread.sleep()instart()— The retry loop with exponential backoff (up to ~30s) blocksCamelContext.start(). A component'sstart()method ideally should not compensate for infrastructure not being ready — that is the deployer's responsibility. Consider failing fast, or lazy cache acquisition on first operation. -
Infinispan IT
RemoteCacheManagerleak — The test creates aRemoteCacheManagerexternally viasetCacheManager()but never closes it (no@AfterAll). SincemanagedCacheManager=false,stop()won't close it either. -
Adoc include directives misplaced —
component-configure-options.adocinclude is inside thecomponent options: START/ENDblock instead of thecomponent-configure options: START/ENDblock. This will cause the generated options tables to render in unexpected locations.
Low
-
Redis/Infinispan
managed*flag not reset instop()— If ownership changes between restart cycles (e.g.,stop(), thensetRedisson(externalClient), thenstart()), the externally provided client gets erroneously shut down on the nextstop(). -
Silent backend discard — When a
storeNamealready has a backend, an explicitly configuredbackend=#myBackendon a new endpoint is silently ignored. Should at least log at WARN level. -
No fallback log — When zero
StateStoreBackendbeans are found in the registry, no log indicates the fallback toInMemoryStateStoreBackend. A DEBUG/INFO message would help troubleshooting. -
InMemoryStateStoreBackend.keys()— Scans expired entries but doesn't evict them, leading to memory accumulation for high-churn short-TTL workloads. -
Redundant caffeine version —
camel-state-store-caffeine/pom.xmlspecifies${caffeine-version}explicitly; it's already managed in parent dependency management. -
Test gaps:
- Redis IT
testDelete()doesn't verify the key is gone after deletion (compare to Caffeine/InMemory tests which do a follow-upget) - Infinispan IT missing explicit
testClear()and negativecontainscase - No test for invalid operation name in header (e.g.,
"BOGUS") - No test for delete of non-existent key (behavior may differ between backends)
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Claus Ibsen
a5f7d3d to
5c70824
Compare
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 597 tested, 23 compile-only — current: 76 all testedMaveniverse Scalpel detected 620 affected modules (current approach: 76).
|
|
Thanks for the thorough review @davsclaus! All 12 findings have been addressed in the latest push. Here's a summary: Critical
Medium
Low
Claude Code on behalf of Guillaume Nodet |
|
@oscerd — the blocking items from your review have also been addressed:
Claude Code on behalf of Guillaume Nodet |
|
CI note: The Java 25 build failure is unrelated to this PR — it's caused by the known |
da481ca to
e0a097c
Compare
|
Claude Code on behalf of gnodet Note: the For now, this PR keeps |
b6f9cb1 to
f003019
Compare
… store Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f003019 to
4e85b7b
Compare
Summary
Claude Code on behalf of gnodet
Adds a new
camel-state-storecomponent that provides a unified key-value store API with pluggable backends. This is useful for caching, session state, and scenarios where you need a simple object store (similar to MuleSoft's Object Store).Modules
camel-state-storecamel-state-store-caffeinecamel-state-store-rediscamel-state-store-infinispanOperations
put,putIfAbsent,get,delete,contains,keys,size,clear— with optional per-entry TTL.Key Design Points
StateStoreBackendbean in the registry is auto-detectedcamel.beans.*properties (no Java required)StateStoreBackendextendsorg.apache.camel.Servicefor proper lifecycle managementChanges since last review
All 12 findings from review #4927079320 plus the version comment have been addressed:
camel-state-store,camel-state-store-caffeine,camel-state-store-redis,camel-state-store-infinispan) instead of just the core moduleStateStoreComponent.doStop()resource leak — eachbackend.stop()is now wrapped in try-catch, logs warning on failure, and rethrows the first exception after attempting all stopsThread.sleepretry loop — removed entire retry loop with exponential backoff fromstart(); now fails fastRemoteCacheManagerleak — addedtestCacheManagerfield with@AfterEach cleanUp()in both IT classescomponent-configure-options,component-endpoint-options, andcomponent-endpoint-headersincludes into their correctSTART/ENDblocksstop()— both backends now resetmanagedRedisson/managedCacheManagertofalseinstop()putIfAbsentand logs WARN when a store name already has a different backendStateStoreBackendfound (falling back to in-memory) and DEBUG log for auto-discoverykeys()andsize()now evict expired entries viaremoveIf()instead of just filtering them${caffeine-version}from caffeine backend pom.xml (managed by parent BOM)@sincetags, and generated filesTest plan
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com