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

Introduce cache injection and NoOpCache #13296

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

gwenneg
Copy link
Member

@gwenneg gwenneg commented Nov 14, 2020

This PR lays some groundwork for the programmatic caching API (#8140). The vast majority of the changes here come from #8631 and has been discussed there already. I recently added NoOpCache which hasn't been reviewed though.

So why create a new PR instead of keeping all this in #8631? For two reasons:

  • I had a talk yesterday with @phillip-kruger about some enhancements (stats for example) he wanted to submit for the cache extension and these enhancements require the ability to inject a cache (which was already developed in Introduce programmatic caching API #8631).
  • Most of this PR content is about internal implementation, it only contains one annotation and two interfaces which will be part of the programmatic API: @CacheName , Cache and CacheManager. I didn't put in this PR any of the API methods from Introduce programmatic caching API #8631 related to cached data (data retrieval or invalidation) since it was still being discussed. That's why the Cache interface is empty for now. @phillip-kruger will probably be the first one to add a method in this interface.

This PR does not change anything in terms of behaviour for the annotations caching API when the extension is enabled. When it is disabled, the only change is the introduction of NoOpCache. The old way to disable the extension at build time wasn't compatible with the programmatic API, hence this new type of cache.

This PR does not contain any change to the doc, that's intentional. I don't think we should document the programmatic API or the ability to inject a cache before there is at least one method in the Cache interface.

I think this can be merged even if #8631 is still a WIP with a few open discussions as long as @CacheName , Cache and CacheManager are accepted as new members of the undocumented caching API (it seemed OK in #8631).

@ghost ghost added the area/cache label Nov 14, 2020
@gwenneg gwenneg force-pushed the issue-8140-cache-injection branch from a52669d to eef3439 Compare November 14, 2020 21:28
@gwenneg
Copy link
Member Author

gwenneg commented Nov 15, 2020

Here are a few code examples to help validating the programmatic caching API elements from this PR:

▶️ Field Cache injection

@ApplicationScoped
public class MyService {

    @CacheName("my-cache") // @Inject can be used but is optional.
    Cache cache;

    public void doSomething() {
        // Use future programmatic caching API method e.g. cache.getStats()
    }
}

▶️ Constructor Cache injection

@ApplicationScoped
public class MyService {

    public MyService(@CacheName("my-cache") cache) {
        // Use future programmatic caching API method e.g. cache.getStats()
    }
}

▶️ Field CacheManager injection

@ApplicationScoped
public class MyService {

    @Inject
    CacheManager cacheManager;

    public void clearAllCaches() {
        // Let's suppose for this example that Cache.invalidateAll() is added in the future
        // (it is not available right now)
        Collection<String> cacheNames = cacheManager.getCacheNames();
        for (String cacheName : cacheNames) {
            Cache cache = cacheManager.getCache(cacheName);
            cache.invalidateAll();
        }
    }
}

@gwenneg
Copy link
Member Author

gwenneg commented Nov 15, 2020

cc @emmanuelbernard @galderz

@gwenneg gwenneg force-pushed the issue-8140-cache-injection branch 2 times, most recently from 9775531 to 35212e5 Compare November 25, 2020 20:00
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added one small question.

*/
@Qualifier
@Retention(RetentionPolicy.RUNTIME)
@Target({ FIELD, METHOD, PARAMETER })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You allow it for methods here but you don't check the method case in the processor. Is it normal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! It is allowed on methods because of CacheProducer but I should add a check in CacheProcessor to forbid the annotation on any other method than the one from the producer.

Copy link
Member Author

@gwenneg gwenneg Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the following changes:

  • rebase on master to fix conflicts
  • new check in CacheProcessor: @CacheName is now only allowed on methods from io.quarkus.cache.runtime.CacheProducer
  • injection tests coverage improvement

Despite the targets of @CacheName and the new check, it is possible to inject a cache using that annotation at all CDI usual injections points: fields, constructors parameters and methods parameters. These are all tested in this PR.

@gwenneg gwenneg force-pushed the issue-8140-cache-injection branch 2 times, most recently from b3b6e0f to 3328885 Compare December 11, 2020 00:02
@gwenneg gwenneg force-pushed the issue-8140-cache-injection branch from 3328885 to ddd8382 Compare December 11, 2020 08:30
@gsmet gsmet merged commit 3f816ad into quarkusio:master Dec 11, 2020
@ghost ghost added this to the 1.11 - master milestone Dec 11, 2020
@gwenneg gwenneg deleted the issue-8140-cache-injection branch December 11, 2020 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants