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

Deprecate and remove Uni<Void> invalidateIf(Predicate<Object> predicate) from Cache API #39901

Closed
karesti opened this issue Apr 5, 2024 · 7 comments

Comments

@karesti
Copy link
Member

karesti commented Apr 5, 2024

Description

As discussed on the linked pull-requests, invalidateIf method is a dangerous and not recommended operation particularly for client/server caching providers such as Infinispan or Redis. The API should be minimal to apply to all caching providers.
The as(Class type) method already gives access to the underlying native cache for additional method calls proposed by the implementation.

#30316
#39836

Implementation ideas

Redis and Infinispan Cache extensions, should not implement this method while is preset in the API, before removal

@karesti karesti added the kind/enhancement New feature or request label Apr 5, 2024
Copy link

quarkus-bot bot commented Apr 5, 2024

/cc @gwenneg (cache)

@karesti
Copy link
Member Author

karesti commented Apr 5, 2024

@mkouba
Copy link
Contributor

mkouba commented Apr 5, 2024

As I stated in #30316 (comment) I don't think we should deprecate/remove this method but clarify that it may not be supported by the underlying implementation and throw UnsupportedOperationException instead 🤷.

@cescoffier
Copy link
Member

It actually can be used with any implementation, as soon as the key set is not too large. Thus, I would recommend keeping the method with a big warning - do not use this method if you key set if very large.

@karesti
Copy link
Member Author

karesti commented Apr 8, 2024

If the fact that no caching provider offers this method doesn't ring a bell to you, I'm not sure what more I can say to convince you.

@karesti karesti closed this as completed Apr 8, 2024
@tristantarrant
Copy link

I think we should go back to the original reason for implementing this method. What was the envisioned use-case ? It feels very artificial. A quick scan of the Quarkus repo doesn't show any uses of it. Neither Spring Cache nor JCache have anything like it. None of the client/server caching implementations have an efficient way of implementing this, unless it's with server-side scripts/tasks.
If the API cannot be implemented efficiently across providers (and I don't see how it could be, since the predicate can be a fully-fledged Java method), then it provides no advantage to an explicit iteration, making it much more obvious to the developer of the involved cost.

@mkouba
Copy link
Contributor

mkouba commented Apr 8, 2024

I think we should go back to the original reason for implementing this method. What was the envisioned use-case ? It feels very artificial.

If I remember correctly the original use case was to be able to invalidate a subset of cache entries for Qute templates, based on the key, see https://quarkus.io/guides/qute-reference#cached-section. Note that {#cached} is not even supported for remote caches.

None of the client/server caching implementations have an efficient way of implementing this, unless it's with server-side scripts/tasks.

As I already mentioned this method got added before there were client/server caching implementations supported.

If there's no efficient way then why not throw UnsupportedOperationException and document that properly?

then it provides no advantage to an explicit iteration, making it much more obvious to the developer of the involved cost.

The advantage is that from client POV this operation is atomic, whereas iterating over keys and then removing some entries is not.

That said, if you really think that io.quarkus.cache.Cache should only declare methods that can be implemented efficiently by both local and remove cache implementations then feel free to deprecate this method and remove it in a future release (3.13+ maybe?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants