Allow to create a CacheManager using an Ehcache builder by henri-tremblay · Pull Request #2347 · ehcache/ehcache3

henri-tremblay · 2018-03-20T12:14:08Z

No description provided.

chrisdennis · 2018-03-20T20:34:03Z

+  public CacheManager getCacheManager(URI uri, ClassLoader classLoader, CacheManagerBuilder<org.ehcache.CacheManager> builder) {
+    Eh107CacheManager cacheManager;
+
+    synchronized (cacheManagers) {


We can probably extract this common block between here and the existing code in to a combination of a computeIfAbsent and a compute.

Yes. I've been purposefully lazy.

chrisdennis · 2018-03-20T20:35:58Z

+  private Eh107CacheManager createCacheManager(URI uri, ClassLoader classLoader, CacheManagerBuilder<org.ehcache.CacheManager> builder) {
+    Eh107CacheLoaderWriterProvider cacheLoaderWriterFactory = new Eh107CacheLoaderWriterProvider();
+
+    Object[] serviceCreationConfigurations = builder.getServices().toArray();


Type safety all messed up here. The code is indeed looking for a ServiceCreationConfigurations but the builder will return services. We'll have to fix the "extra services" logic to catch both services instances and creation configs that would result in service instances. Would be good to have that as common between the two paths as well.

Which means the findSingletonAmongst is strangely permissive and this code doesn't work at all? Or it supports both?

findSingletonAmongst is just conventionally permissive. This is it's signature:

<T> T findSingletonAmongst(Class<T> clazz, Object ... instances)

What we want as an end result is to be certain that the cache manager has a Jsr107Service and the Eh107CacheLoaderWriterProvider. We also need to make sure that any existing config is honored, which I think in this context means:

use the 107 service creation config if it exists in the builder (that should be a natural thing though)

not add a 107 service if there is one already defined

not add the special loader writer factory if there is a loader writer already???

Yep. I agree. Then, for the loader-writer... I'm not sure we actually need a CacheLoaderWriterProvider. We seem to just need something to handle CacheLoader and CacheWriter. The loader-writer is for a cache. So I guess we should ignore it is a CacheLoaderWriterProvider is already provided. If we want to keep things simple.

chrisdennis · 2018-03-20T20:36:30Z

+      services.add(new DefaultJsr107SerializationProvider());
+    }
+
+    InternalCacheManager ehcacheManager = (InternalCacheManager) builder.build(true);


Need to actually add the services to the builder.

Thats services collection doesn't get consumed (unless I'm missing something).

chrisdennis

Changes look good in principle but some details need fixing/clearing up.

henri-tremblay · 2018-03-21T21:34:46Z

I think all changes were implemented.

chrisdennis · 2018-03-29T15:09:25Z

    this.jsr107Cache = ehCache.createJsr107Cache();
  }

+  private StatisticsService getStatisticsService(Eh107CacheManager cacheManager) {


Can be indeed

chrisdennis · 2018-03-29T15:12:37Z

Could we separate this from the exposure of the ServiceLocator so that we can consider the two changes independently?

henri-tremblay · 2018-03-29T15:47:21Z

It's hard (impossible?) because getting rid of Eh107InternalCacheManager makes it possible. Otherwise, I will need a CacheManagerBuilder.build107CacheManager

chrisdennis · 2018-05-15T13:32:43Z

Lets see if we can come to agreement on #2346 then, which would allow this to go through.

henri-tremblay · 2018-07-16T20:03:36Z

@chrisdennis Now rebased on top of #2346 . Can you have a look?

chrisdennis · 2018-07-17T15:56:21Z

+    return getCacheManager(uri, classLoader, () -> createCacheManager(uri, classLoader, builder));
+  }
+
+  private CacheManager getCacheManager(URI uri, ClassLoader classLoader, Supplier<Eh107CacheManager> cacheManagerCreator) {


It feel like this Supplier should really be a function, possibly taking the URI and ClassLoader as arguments.

I tried but it makes things redundant. With lines like return getCacheManager(uri, classLoader, (u, c) -> createCacheManager(u, c, builder));. But I see what you mean. It removes the closure for these two params by the caller. But I still have a closure. Still prefer me to change it?

chrisdennis · 2018-07-17T15:59:06Z

-  private Eh107CacheManager createCacheManager(URI uri, Configuration config, Properties properties) {
-    Eh107CacheLoaderWriterProvider cacheLoaderWriterFactory = new Eh107CacheLoaderWriterProvider();
+  private Eh107CacheManager createCacheManager(URI uri, ClassLoader classLoader, CacheManagerBuilder<org.ehcache.CacheManager> builder) {
+    Set<Service> services = builder.getServices();


This method feels like it contains logic that should really be a part of the CacheManagerBuilder. Fixing this would mean either adding a new method to the builder that only added the service if it didn't already exist, or modifying the contract of the using method to behave more consistently with the using method that take a config object. This would really be moving a failure that will occur anyway when using using from being a CM build time to earlier in the builder invocations (arguably a good thing).

What would you do instead? usingIfNotPresent or using(Service service, boolean overrideIfPresent)? I agree that I don't like the getServices either.

I'm thinking the sensible solution would be to add usingIfAbsent(...) methods to the builder. At the same time it might make sense to also align the contracts for the service and service-config related methods since I think they vary a bit at the moment.

chrisdennis · 2018-08-10T14:02:15Z

Given our proximity to release and the associated need for stabilization I'd like to hold off on merging this until after the release goes out. I'd also like to see if we can align more of the using behaviors like previously discussed.

we are now pass the release

…nted

henri-tremblay added the under review label Mar 20, 2018

chrisdennis reviewed Mar 20, 2018

View reviewed changes

chrisdennis requested changes Mar 20, 2018

View reviewed changes

henri-tremblay force-pushed the cachingproviderbuilder branch from 94d1bd4 to 07631be Compare March 21, 2018 21:33

chrisdennis reviewed Mar 29, 2018

View reviewed changes

henri-tremblay force-pushed the cachingproviderbuilder branch from 07631be to 7792dca Compare May 22, 2018 18:32

henri-tremblay force-pushed the cachingproviderbuilder branch 3 times, most recently from d83920c to 3847f36 Compare July 16, 2018 20:01

chrisdennis previously requested changes Jul 17, 2018

View reviewed changes

henri-tremblay force-pushed the cachingproviderbuilder branch 2 times, most recently from 0ccb0cb to f046974 Compare August 6, 2018 18:47

henri-tremblay force-pushed the cachingproviderbuilder branch 2 times, most recently from f6b4709 to 01711c0 Compare August 29, 2018 18:04

henri-tremblay force-pushed the cachingproviderbuilder branch 2 times, most recently from f70198f to b870a6f Compare September 14, 2018 15:10

henri-tremblay force-pushed the cachingproviderbuilder branch 2 times, most recently from e16fb5f to 2318ff8 Compare December 10, 2018 16:41

henri-tremblay added this to the 3.7.0 milestone Dec 10, 2018

akomakom closed this Dec 10, 2018

akomakom reopened this Dec 10, 2018

AbfrmBlr added under review and removed under review labels Dec 10, 2018

henri-tremblay force-pushed the cachingproviderbuilder branch 5 times, most recently from a218841 to 76d50d9 Compare December 17, 2018 02:33

henri-tremblay added 9 commits January 4, 2019 12:07

Allow to create a CacheManager using an Ehcache builder

4eb764e

Also consider service configuration before setting a default service

0804b42

Fix bad link

0749e28

Make sure we have no CacheManager in CachingProvider when starting test

7e877ec

Make sure we clean the CachingProvider after a test

4b3734f

Store isn't in the same project and so can't be referenced from javadoc

d2fbbb2

sourcedir should be at 36 now

0227595

Push the timeout up a bit and mention the test that failed.

834ed57

Test was flaky. Now we make sure the histogram window changes when wa…

26e98cc

…nted

henri-tremblay force-pushed the cachingproviderbuilder branch from 76d50d9 to 26e98cc Compare January 4, 2019 18:12

henri-tremblay closed this Jan 4, 2019

henri-tremblay reopened this Jan 4, 2019

chrisdennis removed this from the 3.7.0 milestone Feb 16, 2019

Conversation

henri-tremblay commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisdennis left a comment

Choose a reason for hiding this comment

Uh oh!

henri-tremblay commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisdennis commented Mar 29, 2018

Uh oh!

henri-tremblay commented Mar 29, 2018

Uh oh!

chrisdennis commented May 15, 2018

Uh oh!

henri-tremblay commented Jul 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisdennis commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

henri-tremblay commented Mar 20, 2018 •

edited

Loading