gnodet-bot commented on code in PR #28822:
URL: https://github.com/apache/camel/pull/28822#discussion_r4439645327
##########
components/camel-ehcache/src/test/java/org/apache/camel/component/ehcache/EhcacheRemoveRouteTest.java:
##########
@@ -144,42 +144,50 @@ void removeEventRouteThenStopClosesCacheCreatedOnDemand()
throws Exception {
put("direct:plain", "k2", "v2");
assertEquals("v2", get("direct:plain", "k2"));
- // without a configuration the endpoint's own EhcacheManager creates a
user managed cache
+ // without a configuration the endpoint's own EhcacheManager creates a
managed cache on demand
EhcacheEndpoint endpoint = context.getEndpoint("ehcache:plain",
EhcacheEndpoint.class);
- UserManagedCache<?, ?> cache
- = (UserManagedCache<?, ?>)
endpoint.getManager().getCache("plain", Object.class, Object.class);
- assertEquals(Status.AVAILABLE, cache.getStatus());
+ Cache<?, ?> cache = endpoint.getManager().getCache("plain",
Object.class, Object.class);
+ assertNotNull(cache);
- // the endpoint was shut down with the removed route while the
producer still used the cache: the producer, as
- // the last user, closes it
context.stop();
Review Comment:
⚠️ **Test no longer verifies its stated contract.** The test is named
`removeEventRouteThenStopClosesCacheCreatedOnDemand` — the "stop closes cache"
part is what it is supposed to test. The `assertNotNull(cache)` above is the
pre-stop sanity check; after `context.stop()` there is no assertion at all.
davsclaus noted that `org.ehcache.core.Ehcache` (the managed implementation)
still supports `Status`, so a lifecycle check is still possible. For example:
```suggestion
context.stop();
assertEquals(Status.UNINITIALIZED,
endpoint.getManager().getCacheManager().getStatus());
```
Or if the managed `CacheManager` is shared and still open (because other
tests share it), assert on the cache:
`assertNull(endpoint.getManager().getCacheManager().getCache("plain",
Object.class, Object.class))` after stop. Without some assertion here the test
title is misleading.
##########
components/camel-ehcache/src/test/java/org/apache/camel/component/ehcache/EhcacheRemoveRouteTest.java:
##########
@@ -144,42 +144,50 @@ void removeEventRouteThenStopClosesCacheCreatedOnDemand()
throws Exception {
put("direct:plain", "k2", "v2");
assertEquals("v2", get("direct:plain", "k2"));
- // without a configuration the endpoint's own EhcacheManager creates a
user managed cache
+ // without a configuration the endpoint's own EhcacheManager creates a
managed cache on demand
EhcacheEndpoint endpoint = context.getEndpoint("ehcache:plain",
EhcacheEndpoint.class);
- UserManagedCache<?, ?> cache
- = (UserManagedCache<?, ?>)
endpoint.getManager().getCache("plain", Object.class, Object.class);
- assertEquals(Status.AVAILABLE, cache.getStatus());
+ Cache<?, ?> cache = endpoint.getManager().getCache("plain",
Object.class, Object.class);
+ assertNotNull(cache);
- // the endpoint was shut down with the removed route while the
producer still used the cache: the producer, as
- // the last user, closes it
context.stop();
- assertEquals(Status.UNINITIALIZED, cache.getStatus());
+
+ // all endpoints were stopped with the context: the manager reference
count must have reached 0
+ assertEquals(0, endpoint.getManager().getReferenceCount().get());
}
@Test
void producerTakesTheCacheAgainWhenItsManagerWasClosedAndOpenedAgain()
throws Exception {
- EhcacheEndpoint endpoint = context.getEndpoint("ehcache:restarted",
EhcacheEndpoint.class);
- Producer producer = endpoint.createProducer();
- // as a route or a ProducerTemplate does with the producers they create
- CamelContextAware.trySetCamelContext(producer, context);
- producer.start();
- assertNull(send(endpoint, producer, EhcacheConstants.ACTION_PUT, "k1",
"v1").getException());
-
- // the last users stop: the manager closes the cache created on demand
- producer.stop();
- endpoint.stop();
- assertEquals(0, endpoint.getManager().getReferenceCount().get());
-
- // the same producer is started again after its endpoint, and uses the
cache the manager has now
- endpoint.start();
- producer.start();
+ // Register a second component instance so this endpoint gets its own
default CacheManager,
+ // independent of the default one shared by other test routes (which
would keep references
+ // and prevent the manager from closing when the producer and endpoint
below stop).
+ EhcacheComponent ehcache2 = new EhcacheComponent();
+ context.addComponent("ehcache2", ehcache2);
try {
- assertNull(send(endpoint, producer, EhcacheConstants.ACTION_PUT,
"k2", "v2").getException());
- Exchange got = send(endpoint, producer,
EhcacheConstants.ACTION_GET, "k2", null);
- assertNull(got.getException());
- assertEquals("v2", got.getMessage().getBody());
- } finally {
+ EhcacheEndpoint endpoint =
context.getEndpoint("ehcache2:restarted", EhcacheEndpoint.class);
+ Producer producer = endpoint.createProducer();
Review Comment:
🔹 **Stale comment.** This test now uses `cacheManager=#myManager`, which is
a user-supplied `CacheManager` wrapped with `managed=false`. Camel does not
close its caches. The comment "the manager closes the cache created on demand"
was accurate for the old unshared default manager but is no longer correct here.
```suggestion
// the last users release their reference; the user-supplied
cacheManager stays open
producer.stop();
endpoint.stop();
```
--
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]
