CAMEL-23239: Add camel-state-store component with pluggable key-value store - #22158
CAMEL-23239: Add camel-state-store component with pluggable key-value store#22158gnodet wants to merge 1 commit into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
github-actions
Bot
added
components
core
catalog
docs
dsl
core-build-and-dependencies
tooling
tooling-maven
labels
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". |
gnodet
changed the title
|
the commit message also needs to be updated with the correct jira issue number |
apupier
left a comment
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:
|
| [source,java] | ||
| ---- | ||
| @BindToRegistry("infinispanBackend") | ||
| public InfinispanStateStoreBackend infinispan() { |
There was a problem hiding this comment.
@gnodet is this kind of configuration doable only via java beans? would it be possible to provide this configuration via properties?
There was a problem hiding this comment.
Good point! I've pushed an update that addresses this:
Property-based configuration — backends can now be fully configured via application.properties using Camel's camel.beans.* syntax, without writing any Java code:
# Caffeine example
camel.beans.caffeineBackend = #class:org.apache.camel.component.statestore.caffeine.CaffeineStateStoreBackend
camel.beans.caffeineBackend.maximumSize = 50000# Redis example
camel.beans.redisBackend = #class:org.apache.camel.component.statestore.redis.RedisStateStoreBackend
camel.beans.redisBackend.redisUrl = redis://myhost:6379
camel.beans.redisBackend.mapName = my-app-state# Infinispan example
camel.beans.infinispanBackend = #class:org.apache.camel.component.statestore.infinispan.InfinispanStateStoreBackend
camel.beans.infinispanBackend.hosts = myhost:11222
camel.beans.infinispanBackend.cacheName = my-cacheAuto-discovery — if a single StateStoreBackend bean is found in the registry (whether registered via Java or via properties), it is automatically used by all state-store endpoints. No backend=#beanName reference needed:
- route:
from:
uri: direct:store
steps:
- setHeader:
name: CamelStateStoreKey
constant: myKey
- to:
uri: state-store:myStore?operation=putBoth features are covered by new tests (CaffeineStateStorePropertiesTest and StateStoreAutoDiscoveryTest).
Claude Code on behalf of Guillaume Nodet
9acd8bf to
8bbae8d
Compare
| String key = requireKey(message); | ||
| Object value = message.getBody(); | ||
| Object existing = backend.putIfAbsent(key, value, ttl); | ||
| message.setBody(existing); |
There was a problem hiding this comment.
maybe normal but I was surprised when reading it that when performing and operation to store a state, we change the message body with the previous value
There was a problem hiding this comment.
I'm even wondering why modifying the body when setting something in the state store?
There was a problem hiding this comment.
The return-previous-value behavior follows java.util.Map.put() / Map.putIfAbsent() semantics:
put— returns the previous value (ornullif key was new), same asMap.put()putIfAbsent— returns the existing value if key already existed (meaning nothing was stored), ornullif the value was stored successfully
This is intentional — it's the only way to communicate the result back to the route, and it enables patterns like idempotent deduplication:
from("kafka:orders")
.setHeader(StateStoreConstants.KEY, simple("${header.orderId}"))
.to("state-store:processed?operation=putIfAbsent")
.choice()
.when(body().isNotNull()) // non-null means key already existed = duplicate
.log("Duplicate order, skipping").stop()
.end()
.to("direct:process-order");This is consistent with how camel-caffeine-cache handles it (the PUT action also returns the previous value in the body).
Claude Code on behalf of Guillaume Nodet
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
Seems fine but I'm really not an expert on this piece of code and what it applies. i think it would be nice to have someone more experimented reviewing it.
| lastException = e; | ||
| LOG.warn("Failed to access cache '{}' (attempt {}/10): {}", cacheName, i + 1, e.getMessage()); | ||
| try { | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
what about using resilience4j? would it be overkill?
There was a problem hiding this comment.
Good suggestion! I considered it but decided against it to keep the backend module dependency-minimal. resilience4j would add a transitive dependency just for a startup retry loop that only runs once during backend initialization. The exponential backoff loop (now updated from fixed 1s to 200ms–5s backoff) is straightforward and self-contained — it handles the narrow case of Infinispan server startup race conditions.
If the retry logic were more complex (circuit-breaking on operations, bulkhead patterns, etc.), resilience4j would be the right tool, but for a one-time startup sequence I think plain retry-with-backoff is the pragmatic choice.
Claude Code on behalf of Guillaume Nodet
This sounds fine, but from a usability perspective, it would be nice to be able to reuse the existing Camel Infinispan or Redis components. This is not the first time we are "reinventing the wheel" for this kind of use case. Given that we already have Infinispan and Redis components with their own configurations — which are supposed to be more feature-rich than the current state store implementation — I was wondering whether we could find a way to reuse those components (or at least part of their logic) for these use cases. |
I'm failing to see the reason why we should have another component of this kind, it will cause another dose of entropy. |
Pros:
Cons:
Conclusion: So, I am not opposed to have this one on the code base and I think there is potential for making stateful operations/caching a bit more elegant. The consistency of the operations make the behavior a bit more predictable regardless of the backend in use. However, I believe that we should have a greater discussion about what our vision and/or medium term goals for the problem is. Even if the API is elegant, it may not be enough if we don't collectively share the same vision about how we would like this to be ... As such, I'm currently leaning towards 0 on this one and would like to hear more feedback from the community before having a decision on -1 or +1 from my side. |
e5f9441 to
6b8adb7
Compare
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Thanks for this — camel-state-store is a well-structured component with a clean pluggable SPI, thorough docs, and the generated files / DSL wiring are all in place. A couple of blockers before merge:
Blocking
- CI is red: the module's parent is still
org.apache.camel:components:4.19.0-SNAPSHOTwhilemainis now4.21.0-SNAPSHOT, so the-Pregenbuild can't resolve the parent POM. A rebase onmainshould fix this and also refresh the stale4.19.0markers (the@UriEndpoint(firstVersion=...), the four module poms, and:since: 4.19in the docs), and re-run the generated files. - The new tests use
Thread.sleep()for the TTL-expiry waits (e.g. inStateStoreTtlTest,CaffeineStateStoreBackendTest, and the Infinispan/Redis ITs). Per the project's testing guideline these should move to Awaitility (await().atMost(...).untilAsserted(...)), which is also less flaky on slow CI agents.
Minor / question
- A couple of files use the FQCN form
@org.apache.camel.spi.annotations.Component(...)andjava.util.Map.of(...)rather than imports — OpenRewrite normally normalizes this, but the regen step fails before that phase. MojoHelper.getComponentPath()listscamel-state-storebut not the three backend modules (caffeine/redis/infinispan). The generated files are committed so it evidently worked, but it may be worth registering them the waycamel-infinispandoes — could you confirm?
Solid contribution overall — the asks here are mostly mechanical.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
- Add check-pr-work.sh: lightweight bash precondition (1 API call, no LLM tokens) that checks for actionable PRs before running the expensive review loop. Exits 1 to skip empty iterations. - Remove diff size limit: review all PRs regardless of size (was 2000 lines) - Remove apache#22158 from Skipped table (was skipped for being too large) - Update SKILL.md, CLAUDE.md, and loop-constraints.md accordingly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code self-review on behalf of @gnodet
Self-review: CAMEL-23239 — Add camel-state-store component (re-review after 6 new commits)
Well-designed component with clean SPI, smart auto-discovery, comprehensive docs, and good test coverage. The PR description is exemplary. However, several mechanical issues need to be fixed before this can merge.
Blocking Issues
1. Stale version 4.19.0-SNAPSHOT (already flagged by @oscerd)
All POM files, @UriEndpoint(firstVersion), doc :since: tags, and generated JSON files reference 4.19.0-SNAPSHOT while main is at 4.22.0-SNAPSHOT. Needs a rebase and version update.
2. Thread.sleep() in tests and production code (already flagged by @oscerd)
8 occurrences across test files, plus one in production code:
InfinispanStateStoreBackend.start()line ~111:Thread.sleep(1000)in a 10-retry loop — this blocks the thread for up to 10s during startup. Consider exponential backoff or Camel's retry mechanism.- All TTL expiry tests use
Thread.sleep(1000)orThread.sleep(2500)— should use Awaitility per project conventions.
3. FQCN usage in non-generated code
InMemoryStateStoreBackend.java:.map(java.util.Map.Entry::getKey)→ importMap.EntryStateStoreComponent.java:@org.apache.camel.spi.annotations.Component("state-store")→ import and use@Component
Important (non-blocking)
4. StateStoreBackend doesn't implement Camel's Service interface
The SPI defines its own start()/stop() instead of extending org.apache.camel.Service. Extending Service would let ServiceHelper manage backends and enable Camel lifecycle state tracking. This is an intentional decision (SPI not in camel-api), but worth documenting in the interface Javadoc.
5. Backend lifecycle race in getOrCreateBackend()
When two endpoints with the same storeName but different explicit backends race, only the first wins silently. No warning is logged. Also, registry auto-discovery inside computeIfAbsent could theoretically deadlock with lazy bean creation.
6. Redis redisUrl may contain credentials — not marked secret
redisUrl can embed auth credentials (redis://user:password@host:6379). While backends are configured via camel.beans.* syntax (not @UriParam), recommend documenting that credentials can be embedded and suggesting vault integration for production.
7. Missing @since tags on public classes
StateStoreBackend, StateStoreConstants, StateStoreOperations, etc. should have @since 4.22 (or whatever version after rebase) for consistency.
Minor
InMemoryStateStoreBackend.putIfAbsent()usesObject[]capture —AtomicReferencewould be cleanersize()andkeys()are O(n) with TTL filtering — documented design choice but worth notingInfinispanStateStoreBackendretry loop: swallows intermediate exceptions, only throws the last one- MojoHelper registers only
camel-state-store, not backend modules — this appears correct (backends are "other", not components) but confirm per @oscerd's question
Positives
- ✅ Clean, minimal SPI (9 methods) — appropriate for key-value abstraction
- ✅ Smart auto-discovery: single backend in registry used automatically
- ✅ Per-entry TTL with endpoint default + per-message header override
- ✅ Consistent
putreturns-previous-value semantics across all backends - ✅ Backend sharing via
storeName-keyed map in component - ✅ Comprehensive documentation (430+ lines with Java/YAML DSL tabs)
- ✅ 50+ tests across all modules
Recommendation: Address the 3 blocking issues (rebase, Thread.sleep, FQCNs) — all mechanical fixes. The component design and functionality are solid and ready for merge after that.
6b8adb7 to
f272f0e
Compare
|
Thanks for the thoughtful feedback from @Croway, @oscerd, and @orpiske on the overlap question. I've updated the PR description with a detailed section addressing this, but let me summarize the key points here: Why a new component instead of reusing existing onesThe existing Camel components (caffeine-cache, infinispan, spring-redis) are technology-specific — they expose the full API of each product (queries, pub/sub, statistics, events, etc.). That's their strength, but it's also their overhead when you just need The primary motivation is MuleSoft migration. MuleSoft's Object Store is a core primitive that MuleSoft users rely on heavily. When migrating MuleSoft flows to Camel, there's no direct equivalent — users are forced to pick a technology (Caffeine? Redis? Infinispan?) and learn its full component API, when all they need is a simple key-value store. What the backend modules do (and don't do)The backend modules ( This follows established Camel patterns
Regarding @orpiske's point about medium-term visionI agree that a broader discussion about stateful operations in Camel would be valuable (and CAMEL-11114 "Create cache DSL" is related). This component is designed to be compatible with a future direction: the I've pushed an update addressing all the code-level review feedback (rebased, squashed, versions fixed, Thread.sleep → Awaitility, AssertJ, @SInCE tags, FQCN fixes). Happy to discuss the architectural direction further. Claude Code on behalf of Guillaume Nodet |
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
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
876c9a0 to
a5f7d3d
Compare
… store Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a5f7d3d to
5c70824
Compare
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 597 tested, 23 compile-only — current: 70 all testedMaveniverse Scalpel detected 620 affected modules (current approach: 70).
|
8 participants
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