Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JCache (JSR 107) Extension #3128

Closed
anadollu opened this issue Jul 6, 2019 · 26 comments
Closed

JCache (JSR 107) Extension #3128

anadollu opened this issue Jul 6, 2019 · 26 comments
Assignees
Labels
area/cache kind/enhancement New feature or request triage/duplicate This issue or pull request already exists

Comments

@anadollu
Copy link

anadollu commented Jul 6, 2019

Description
The JCache JSR (JSR-107) Quarkus Extension

Implementation ideas
One of Infinispan or Hazelcast implementations will be enough on the first step.

@anadollu anadollu added the kind/enhancement New feature or request label Jul 6, 2019
@loicmathieu
Copy link
Contributor

+1 but I'd rather love to see an EhCache implementation as it's a widely used cache implementation with a nice API and a lot of people use it.
Maybe we need a JCache implementation where we can plugged in multiple cache provider so we can cover more use cases:

  • local cache (EhCache are the most used)
  • distributed caching (with JGroups for ex)
  • remote caching (client/server like Hazelcast and Infinispan)
  • ...

@gwenneg
Copy link
Member

gwenneg commented Jul 13, 2019

@gsmet Is there already someone working on such a cache extension?

I'd be very interested in contributing to Quarkus by submitting a new extension (it would be my first one) but I'm not a cache expert (only a regular user) so I'm wondering if this subject would be a good one.

@gsmet
Copy link
Member

gsmet commented Jul 13, 2019

I think it would be an interesting addition.

Our choice of implementation would be Infinispan I suppose as we find it to be a good compromise for local and remote caching.

What would be interesting too would be to have caching annotations to automatically cache the result of a bean method - similar to what Spring has with the @Cacheable annotation.

/cc @Sanne

@anadollu
Copy link
Author

@gsmet JSR 107 already defines how the cache abstraction should be. And there are JCache Annotations and Events already defined. Also there are many local or remote JCache provider implementations (Infinispan, hazelcast, eh-cache). Because of that, i suggested JSR107 extension instead of specific cache vendor extension. In my opinion we should stick with the spec and the first provider supported can be Infinispan.

JCache Annotations.
javax.cache.annotation.CacheDefaults
javax.cache.annotation.CacheKey
javax.cache.annotation.CacheResult
javax.cache.annotation.CacheValue
javax.cache.annotation.CachePut
javax.cache.annotation.CacheRemove
javax.cache.annotation.CacheRemoveAll

@gsmet
Copy link
Member

gsmet commented Jul 14, 2019

Ah I didn’t know JCache had a set of annotations, I only used the cache abstraction in the Hibernate ORM context.

I agree with everything you said.

@gsmet
Copy link
Member

gsmet commented Jul 14, 2019

@anadollu do you plan to work on this or can @gwenneg give it a go?

@loicmathieu
Copy link
Contributor

loicmathieu commented Jul 15, 2019 via email

@gsmet
Copy link
Member

gsmet commented Jul 15, 2019

@gwenneg I think you can start on this if you want.

My proposal would be to do it step by step:

  • implement the interceptor layer based on the JCache annotations backed by Infinispan: I think this one only would already be a quite significant work
  • see if we can configure the caches via Quarkus: note that the configuration exposed by JCache is quite limited: you can't set a limit to the number of objects cached for instance
  • then see how we could support plugging other implementations: there's a good chance we will only support one implementation in Quarkus core itself but we plan to push a Quarkus platform with user contributed extensions.

Probably good to share an early branch so that other interested people can step up and help.

@gwenneg
Copy link
Member

gwenneg commented Jul 15, 2019

@gsmet Ok I'll start working on this in the next days and share a branch this next week.

@gwenneg gwenneg self-assigned this Jul 15, 2019
@gsmet
Copy link
Member

gsmet commented Jul 15, 2019

@gwenneg take a look at the Hibernate Validator extension to know how to deal with interceptors.

@Sanne
Copy link
Member

Sanne commented Jul 15, 2019

N.B. the Quarkus team is aiming for an "opinionated" approach, so that we can focus on a smaller set of moving parts in terms of dependencies and actually support an high quality integration among all dependencies.
While it's ok to make as many extensions as you want outside of the "core Quarkus" repository, we will want to pick (and recommend, document, etc..) a single implementation for each use case.

Take this also as a way for us that we won't be able to shift blame of any bugs to the implementation maintainer, we better make really good choices ;-)

That said I agree that we could have a different implementation per each use case as @loicmathieu suggested, quoting him:

local cache (EhCache are the most used)
distributed caching (with JGroups for ex)
remote caching (client/server like Hazelcast and Infinispan)

however since we already discussed this (and work has already started on these) they will be:

  • Caffeine based for local caches
  • Infinispan based for client/server caches
  • no "distributed caching" at the moment - we'll see if there is more demand, and in case it will be Infinispan based too.

cc/ @emmanuelbernard @galderz @tristantarrant

@gwenneg if you want to start working on this, that's awesome! But I would love if you could please focus on local caching, using Caffeine for the first iteration ?

@gwenneg
Copy link
Member

gwenneg commented Jul 15, 2019

@Sanne Ok, I'll do that.

@ben-manes
Copy link

fyi,

JCache annotations have quirks you should be aware of before using.

  • Mostly incompatible with other JCache features (CacheWriter, CacheLoader, CacheEntryListener, etc).
  • Allows for query storms on a cache miss (@CacheResult is a non-blocking load)
  • Silently creates anonymous, unbounded caches if the cache name was not configured explicitly.

These can be worked around, but may require reimplementing parts of the spec. JCache is useful for integrations but can also be fairly limiting. For application code, I recommend using a native caching library's APIs instead (biased as author of Caffeine) as more straightforward and flexible. Of course, integration into a framework like Hibernate, JCache is nice since any provider can be plugged in for basic support.

@Sanne
Copy link
Member

Sanne commented Jul 15, 2019

hi @ben-manes , great to see you commenting as well!

I totally agree. In fact I tried to push back on JCache during a previous design meeting, and discussed such limitations with @galderz - he's the caching expert on the team: implemented the Hibernate caching layer w/o using JCache but rather using a thin layer from Infinispan code as adapter (and to reuse some welltested code we already had), but mostly based on Caffeine.

But since there seems widespread interest in such annotations for general purpose usage, or perhaps something similarly easy to use, I don't feel like pushing back entirely on this "we want JCache" initiative.

Best outcome would be to find a good compromise: not necessarily constrained by all spec details - at least in first iterations - but using it to possibly advance the standards. Not sure exactly how to get there but those points you mentioned certainly bother me.

Regarding the query storms problem specifically; I haven't had the time to work on this but I was thinking that we should be able to make this a blocking load? Consider we'll base our implementation on Caffeine, I really think that having the ability to swap implementation is a non-goal (especially for such reasons).

thanks!

@ben-manes
Copy link

Thanks @Sanne. I agree with not blocking users from getting their work done, and if that means supporting JCache then that's great. I was mostly meaning to warn users from expecting too much from it, as I think it is mostly bad for application code. That doesn't mean not supporting it here, just perhaps not evangelizing it when added. JCache is great for simple integrations between frameworks.

JCache has an odd design choice where implementation details are explicitly stated within the JavaDoc / GoogleDoc. This means certain decisions that should be provider-specific have to emulate the lead's implementation (directly based on Ehcache). There is a bit of leeway with @CacheResult as it explicit says the steps is Cache#get followed by a compute, but I think that compute could be done in a Cache#invoke without violating the spec (but may fail the TCK due to its brittleness). The request to generalize the spec or strengthen the RI to be atomic CacheResult was rejected (jsr107/RI#45).

@tristantarrant
Copy link

There are multiple issues at stake here ranging from the technical to the legal.

First off, I think we should avoid the JCache annotations if possible, as it is unlikely they can be evolved to support additional use cases. We should opt either for a Quarkus-specific solution (low effort, more freedom) or for something more ambitious (Microprofile Caching ?).
As @ben-manes says, we can still support JCache, but only through a compatibility layer rather than having it influence our implementation choices.

Aside from the "locking get" (I also happen to dislike Spring Cache's use of the term "synchronized" here) a useful @Cacheable annotation should also have a read-timeout (as caching may not always be local) as well as encouraging non-blocking patterns (although simply detecting that the annotated method returns a CompletionStage should be enough).

@galderz
Copy link
Member

galderz commented Jul 22, 2019

As seen from the comments above, there's a general consensus that we'd want to have annotation-drive n caching for application data, but JCache falls short here. With this in mind, I've created a separate issue (#3306) where we can decide what these annotations should look like, their behaviour and expectations. For #3306, we want to build the minimum set of annotations with the least amount of configuration to enable applications to cache their own data easily.

@galderz
Copy link
Member

galderz commented Jul 22, 2019

@gwenneg, since you were starting to work on this issue, I've assigned #3306 to you so that you can direct your efforts in that direction.

@gwenneg
Copy link
Member

gwenneg commented Jul 22, 2019

Thanks @galderz and thank you all for your advice and ideas. It's really great having so many caching experts involved in this subject!

@gwenneg
Copy link
Member

gwenneg commented Jul 22, 2019

@gsmet I've created an extension locally with a temp name, but it would be nice if I could rename it with its final name. Do you have any idea about this?

@stale
Copy link

stale bot commented Nov 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you!
We are doing this automatically to ensure out-of-date issues does not stay around indefinitely.
If you believe this issue is still relevant please put a comment on it on why and if it truly needs to stay request or add 'pinned' label.

@stale stale bot added the stale label Nov 13, 2019
@machi1990
Copy link
Member

PR #3394 is still open.

@stale stale bot removed the stale label Nov 13, 2019
@gwenneg gwenneg added the pinned Issue will never be marked as stale label Nov 13, 2019
@gsmet
Copy link
Member

gsmet commented Nov 13, 2019

Closing this in favor of #3306 . No need for 2 issues.

@gsmet gsmet closed this as completed Nov 13, 2019
@gsmet gsmet added the triage/duplicate This issue or pull request already exists label Nov 13, 2019
@gwenneg gwenneg removed the pinned Issue will never be marked as stale label Nov 13, 2019
@machi1990
Copy link
Member

Did not notice the other issue. Let's close :-)

@Sanne
Copy link
Member

Sanne commented Nov 14, 2019

N.B. for the record the other issue isn't aiming at JSR-107 compliance. But I'm fine closing this one, as I was recommending against it: as I explained during design meetings, the spec is enforcing several aspects which are not really optimal nor "modern" anymore.

@gsmet
Copy link
Member

gsmet commented Nov 14, 2019

Thanks for the precision, will be useful if someone stumbles upon this particular issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cache kind/enhancement New feature or request triage/duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

9 participants