Croway commented on PR #29479: URL: https://github.com/apache/camel/pull/29479#issuecomment-6741353034
Thanks for working on this. I reproduced each point below with tests on the PR head (d04045f), using the parent commit (cc2c082) as the control. The Cache EIP and state-store both shipped in 4.23.0, so points 1 and 2 change behaviour for existing users. **1. The documented Cache EIP invalidation recipe no longer works** ([`CacheProcessor.java#L72`](https://github.com/apache/camel/blob/d04045fb40cb0c4f7747cfbef7c26a9d5bd87600/core/camel-core-processor/src/main/java/org/apache/camel/processor/CacheProcessor.java#L72)) Cache entries are now stored as `cache:<key>`. The "Invalidating from Another Route" recipe in `cache-eip.adoc` calls `delete(productId)` on the shared repository, in the Java, XML and YAML examples. That call now does nothing, so the stale value is served until the TTL runs out, or forever when there is no TTL. I ran the Java recipe as written: cache product 42, invalidate it, read it again. | | backend keys after the delete | body after invalidation | |---|---|---| | parent commit | `[]` | `v2` (reloaded) | | PR | `[cache:42]` | `v1` (stale) | The PR doesn't update `cache-eip.adoc`, and no test covers the Cache EIP namespacing. **2. Data written by 4.23 is no longer visible after the upgrade** ([`StateStoreComponent.java#L68`](https://github.com/apache/camel/blob/d04045fb40cb0c4f7747cfbef7c26a9d5bd87600/components/camel-state-store/camel-state-store/src/main/java/org/apache/camel/component/statestore/StateStoreComponent.java#L68), [`CacheProcessor.java#L72`](https://github.com/apache/camel/blob/d04045fb40cb0c4f7747cfbef7c26a9d5bd87600/core/camel-core-processor/src/main/java/org/apache/camel/processor/CacheProcessor.java#L72)) `state-store:orders` key `k1` used to be stored as backend key `k1`; with this PR it is stored as `statestore:orders:k1`. On a persistent backend (Redis, JDBC, Hazelcast, Infinispan, Kafka, Cassandra), the entries already there become invisible to the routes: | `state-store:orders`, backend already holds `k1=v1` | parent commit | PR | |---|---|---| | `get(k1)` | `v1` | `null` | | `size` | `1` | `0` | | backend keys after `clear` | `[]` | `[k1]` (orphaned) | The Cache EIP has the same problem. An entry cached as `43` before the upgrade is ignored and computed again. Without a TTL, the old `43` then stays next to `cache:43` forever: backend keys `[43, cache:43, cache:44]`, where the parent commit returns the cached value and holds `[43, 44]`. The upgrade guide entry only mentions external tools, and there is no way to opt out. The namespace is also applied to backends that already belong to a single store: those from `StateStoreBackendFactory`, and the default per-store `MemoryKeyValueRepository`. **3. Store names that share a prefix are not isolated** ([`NamespacedKeyValueRepository.java#L185`](https://github.com/apache/camel/blob/d04045fb40cb0c4f7747cfbef7c26a9d5bd87600/core/camel-support/src/main/java/org/apache/camel/support/NamespacedKeyValueRepository.java#L185)) Store names can contain `:`, and the namespace isn't escaped, so `statestore:orders:` is also a prefix of `statestore:orders:archive:`. With `state-store:orders` and `state-store:orders:archive` on one backend: - `keys` on `orders` returns `[archive:x, y]`, and `size` returns `2` - a `put` of `archive:x` on `orders` overwrites `x` in `orders:archive` (`get(x)` there returns `overwritten-by-orders`) - `clear` on `orders` deletes the `orders:archive` entries (`get(x)` there then returns `null`) Rejecting `:` in store names, or escaping the store name in the namespace, would make the isolation hold. **4. Generated files are missing or stale** This is why `build (21, false)` is red: its "Fail if there are uncommitted changes" step lists: - the regenerated `NamespacedKeyValueRepositoryConfigurer.java` (it now uses `toLowerCase(Locale.ROOT)`) - the catalog `beans.properties` entry - the untracked `catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/beans/NamespacedKeyValueRepository.json` A local build of `camel-support` regenerates the configurer the same way. **Suggestion:** namespace only when the backend is actually shared (auto-discovered, or set with `backend=#ref`), or add an option to turn it off. Since state-store now namespaces its own keys, the Cache EIP could probably drop the `cache:` prefix. That avoids point 1 and most of point 2. Whatever stays should be documented in `cache-eip.adoc`, `state-store-component.adoc` and the upgrade guide, with a migration note. <details><summary>Reproducer tests</summary> state-store (`CamelTestSupport`; a single `MemoryKeyValueRepository` named `shared` is bound in the registry and auto-discovered; the routes are `from("direct:<op>").toD("state-store:${header.reproStore}?operation=<op>")`): ```java @Test void dataWrittenBy423IsStillReadableAfterUpgrade() { shared.put("k1", "v1"); // what 4.23 wrote for state-store:orders key k1 Object value = template.requestBodyAndHeaders("direct:get", null, Map.of("reproStore", "orders", StateStoreConstants.KEY, "k1")); assertThat(value).isEqualTo("v1"); } @Test void storeNamesSharingAPrefixAreIsolated() { template.requestBodyAndHeaders("direct:put", "archived", Map.of("reproStore", "orders:archive", StateStoreConstants.KEY, "x")); template.requestBodyAndHeaders("direct:put", "live", Map.of("reproStore", "orders", StateStoreConstants.KEY, "y")); Set<String> keys = (Set<String>) template.requestBodyAndHeaders("direct:keys", null, Map.of("reproStore", "orders")); assertThat(keys).containsExactly("y"); } ``` Cache EIP (`ContextTestSupport`; `productCache` is a `MemoryKeyValueRepository` bound in the registry): ```java from("direct:getProduct") .cache(simple("${header.productId}")).keyValueRepository("productCache") .process(e -> e.getMessage().setBody("v" + calls.incrementAndGet())) .end(); // verbatim from cache-eip.adoc from("direct:invalidate") .process(exchange -> { String productId = exchange.getIn().getHeader("productId", String.class); KeyValueRepository cache = exchange.getContext().getRegistry() .lookupByNameAndType("productCache", KeyValueRepository.class); cache.delete(productId); }); @Test void documentedInvalidationRecipeDropsTheCachedEntry() { assertEquals("v1", template.requestBodyAndHeader("direct:getProduct", null, "productId", "42")); template.sendBodyAndHeader("direct:invalidate", null, "productId", "42"); assertEquals("v2", template.requestBodyAndHeader("direct:getProduct", null, "productId", "42")); } ``` </details> _Claude Code on behalf of Croway_ -- 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]
