fix comment about the cache size and clarify configuration

[cruftex]

The comment Create a cache using infinite heap. is wrong.
The JCache standard only requires a cache to store at least "some" values to pass the TCK. It is not required to be heap or infinite. cache2k limits to 2000 entries by default and passes the TCK.

[snicoll]

Thanks for the PR, this is good stuff. I've added a couple of comments.

[snicoll]

I am not sure that's a valid alternative now you've removed the previous content. The intent of the original description was to mention that this was configuring a cache in-memory.

I agree the "infinite heap" part is irrelevant. At the time I meant to focus on the fact that using EhCache's specific API allows you to restrict the size in heap and various other options.

Can you please rephrase the description to focus on the original intent (that this configuration does not configure any sort of persistence for the cache).

[snicoll]

That and the (yet limited) standard API.

[cruftex]

@snicoll, from the PR annotation:

The intent of the original description was to mention that this was configuring a cache in-memory.

That was your intent and this is in fact what is happening when you use EHCache (or Caffeine, or cache2k) without further configuration. But that intent is not and cannot be reflected in the configuration.

The configuration here is generic to any JCache, which may be in heap, off heap or persistent. The comments and implications must therefor make sense for every JCache implementation.

Can you please rephrase the description to focus on the original intent (that this configuration does not configure any sort of persistence for the cache).

That's correct, but it does not configure an in heap cache as well ;)

I thought about mentioning that this yields an in memory cache together with EHCache dependency when no additional configuration is present, but explaining EHCache semantics in a generic JCache configuration file feels very weird.

IMHO, it must be crisp and fit for every JCache. Thoughts?

[snicoll]

IMHO, it must be crisp and fit for every JCache. Thoughts?

Yeah I agree. So perhaps we can focus the description on the statistics part then and mention that additional configuration options are available in this API and the JSR-107 implementation API ?

[cruftex]

Preparing a updated PR which will be actually not so crisp any more but explain all the implications in it.

[snicoll]

Cool, please push force on your existing branch to update this PR.

[cruftex]

Sure thing!

[cruftex]

@snicoll: I did push a very much extended version yesterday.

It's quite detailed now, and the documentation is more than the code. But I think that's how an example should look like ;)

I plan to walk through the other examples and the documentation and propose some updates as well.
Probably, some parts of what I have written here now, make sense in the documentation as well, and/or, the code example should have links to the relevant documentation sections and vice versa.

[snicoll]

It's quite detailed now, and the documentation is more than the code. But I think that's how an example should look like ;)

Sorry but I disagree. That's very useful information for sure but I consider it overwhelming considering what that class does. It's not really the purpose of petclinic to explain how JCache works or some (rather advanced) concepts like what happens if a persistent cache is used.

If you have a couple of links that provides more information I am happy to revisit the PR and use those links instead.

Thoughts?

[cruftex]

It's not really the purpose of petclinic to explain how JCache works or some (rather advanced) concepts like what happens if a persistent cache is used.

Fair point. The stuff is important to know in the context, but this is not the place to do it in that detail.

If you have a couple of links that provides more information I am happy to revisit the PR and use those links instead.

The JCache Spec is a PDF which is not directly linkable (licensing....).

Let's keep the PR dangling. Maybe a few sentences make sense to add in the Spring documentation itself, which is, BTW, a great introduction to all sorts of programming concepts. Then we link there.

Thoughts?

Studying the Spring code, I think the best would be to remove the need of createCache at all and make it default in the Spring JCacheManager. Like so:

	@Override
	protected Cache getMissingCache(String name) {
		CacheManager cacheManager = getCacheManager();
		javax.cache.Cache<Object, Object> jcache = 
                   cacheManager.createCache(name, new MutableCacheConfiguration());
		return new JCacheCache(jcache, isAllowNullValues());
	}

This would make the whole class redundant.

I think it would be great to be able to use the JCache caches as catch all as well.

Of course, now there are the backwards compatibility issues to consider....

[snicoll]

We've had that discussion before and I can't find back the thread but creating the caches on the fly is not happening. We have to fail when a cache is not present so that users know they have to do something. And creating something with a default config doesn't sound a sane choice to me.

This thread is starting to expand to various topics and I'd like it to be focused. I am happy to review a PR to improve the Spring Framework documentation as long as it stays focused on what we do.

[cruftex]

This thread is starting to expand to various topics and I'd like it to be focused.

Thanks for the explanation and sorry for veering off. Maybe the typical "warmup phase".

Since this got too lengthy, I am closing this PR for now and will do a fresh one, once I have the time and gained more oversight.

[snicoll]

No worries. Do you mind if I take bits and pieces of your PR to improve that class. It certainly would be better than the current situation!

[cruftex]

Maybe that's the faster option.

You could also accept the PR and then strip it as you like. I didn't mean to do a hostile take over of this class ;)

[snicoll]

Thanks a lot @cruftex ! I am happy to continue the discussion on the Spring Framework's tracker for further ideas to improve the doc.