Re-introduce Caching #2393
Re-introduce Caching #2393
Conversation
Implementation uses ehCache 3.9. Added test for CohortDefinitionService caching.
Increased size of small-heap configuration.
Fixed permission tests.
Changed asset cache key to use login always due to read/write permissions being assigned at a per-user basis.
… (old style) when caching is disabled.
Changed editor config to tabs. Made cache endpoints auth restricted.
|
@amarsan , would you be willing to do a review on this. I changed one of the functions of the clear cache such that the clear cache button now just clears the WebAPI cache, and the individual 'refresh' buttons next to sources will clear the caches on the specific cdms. We could introduce a button somewhere for refreshing all cdm caches, but I seriously doubt anyone would need that function. The cache endpints do not have a UI to access them, but they exist so that you can fetch the cache statistics as a rest endpoint. However, this requires passing an auth header into the request so that it passes authetnication. To do this, I access Atlas and go to the console to look at one of the secured WebAPI requets (like source/sources). I look at the header and get the authorization header that looks like this: I then use something like postman to make the GET request with the specified header. I think there is also a chrome extension that will let you set headers so that you can make the GET request directly in a browser tab. I'm just giving you this info in case you wanted to observe the cache statistics changing over time. |
| } | ||
| @CacheEvict(cacheNames = CachingSetup.SOURCE_LIST_CACHE, allEntries = true) | ||
| public void invalidateCache() { | ||
| } |
There was a problem hiding this comment.
I see that you have @CacheEvict here, but also on the methods in the SourceController that call this method. Is that redundant?
There was a problem hiding this comment.
I don't think it's redundant, but I did have trouble understanding where the line between the service and the controller was being drawn, so there were certain places where cache evict happens at the controller level and the service level in others. In this function, there was a service-level function to invalidate the cache so that's where the evict annotation applies here. I think the reason why it's at the service level is that the service components are typically the ones that are dependency-injected into other components, so if some other component needs to signal a cache invalidation for some reason, it would call out to this service method.
I'm glad you're poking at this because it is revealing some of the inconsistency and coding confusion that we have in the codebase that we could try to address when we have more flexibility to change things (Ie 3.x and 4.x)
| @@ -157,6 +157,7 @@ public SourceDetails getSourceDetails(@PathParam("sourceId") Integer sourceId) { | |||
| @POST | |||
| @Consumes(MediaType.MULTIPART_FORM_DATA) | |||
| @Produces(MediaType.APPLICATION_JSON) | |||
| @CacheEvict(cacheNames = SourceService.CachingSetup.SOURCE_LIST_CACHE, allEntries = true) | |||
There was a problem hiding this comment.
Does this invalidate the cache even if this is called when security is disabled?
There was a problem hiding this comment.
Yes, the caching is independent of security being enabled.
There was a problem hiding this comment.
Just so I understand how this CacheEvict works...does it execute after the method executes, and only if it executes without throwing an exception?
There was a problem hiding this comment.
It will fire after the method, and if the method throws an exception, it will not evict.
It can be set to evict the cache before invoking the method by using the beforeInvocation option on the annotation.
In this case, I think we want the default behavior, if it fails to create the source for some reason, then there is no need to cache evict.
…caching # Conflicts: # src/main/java/org/ohdsi/webapi/service/ConceptSetService.java
Implemented caching for cohort def list, concept set list, permissions.
Implementation uses ehCache 3.9.
Added test for CohortDefinitionService caching.
Notes:
This implementation diverges from the sandbox: I decided to leverage
@CacheEvicton methods instead of JPA Entity Listeners to simplify the implementation.Currently getting cache info is unprotected, but cache/clear is protected.