[MGNLPN-189] Remove personalisation-specific cache configuration Created: 08/Oct/14  Updated: 22/Jan/15  Resolved: 07/Nov/14

Status: Closed
Project: Magnolia Personalization
Component/s: Integration
Affects Version/s: None
Fix Version/s: 1.1

Type: Task Priority: Neutral
Reporter: Roman Kovařík Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: cache
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
dependency
depends upon MGNLCACHE-57 Avoid unnecessary cache locking for u... Closed
supersession
supersedes MGNLPN-186 Update cache-related update tasks to ... Closed
supersedes MGNLPN-112 Bypass cache for personalized pages Closed
Template:
Acceptance criteria:
Empty
Task DoR:
Empty
Date of First Response:

 Description   
  1. Update dependency to cache 5.3
  2. Add update task to use default cache configuration classes again.
  3. Deprecate classes which were merged into cache:
  • info.magnolia.personalization.cache.BypassUncacheableEntriesPolicy
  • info.magnolia.module.cache.filter.UncacheableEntry
  • Instead of info.magnolia.personalization.cache.BypassVariantsCacheStore, it should be enough to set "Cache-control" header to "no-cache" e.g. in VariantResolver filter if it detects a variant.


 Comments   
Comment by Magnolia International [ 09/Oct/14 ]

Am not convinced about the change to the VariantResolverFilter and the removal of BypassVariantsCacheStore

  • to keep flexibility, there should probably be a visitor pattern for Store implementation or something (so that other modules can contribute their logic), or the logic we have in Store there should be moved to the CachePolicy somehow ? (which means that until we have that, your removing the p13n-specific Store is a good thing)
  • I always prefer an explicit call to something like "is the node a variant, if so then we're uncacheable" rather than setting a header somewhere and as a side-effect we end up not caching the page. It's a matter of taste, so no strong argument, and in this case it's pretty explicit
  • But here's the argument that's actually worth double checking: won't setting that header prevent browser caching too ? All we want (AFAIK) is bypass the server-side cache, and let the browser decide if it wants to cache the page (or influence it by other means)
Comment by Roman Kovařík [ 09/Oct/14 ]

...or the logic we have in Store there should be moved to the CachePolicy somehow ?

I tried that or just create voter for that but the problem is that we don't have the necessary information in AggregationState yet. All what we have is just URI.

I always prefer an explicit call to something like "is the node a variant, if so then we're uncacheable" rather than setting a header somewhere and as a side-effect we end up not caching the page.

I see your point. I wanted to get rid of p13n specific configuration at all because if you'd need a more specific store, you'd need to implement one for CE and one for EE.

won't setting that header prevent browser caching too?

It does . I don't see it as a big problem as:

  1. dontCachePages voter is usually active anyway.
  2. Do we even want to do that? (it would be maybe good to cache a country variant but probably not a new/recurring visitor variant.
    OR we could http://jira.magnolia-cms.com/browse/MGNLCACHE-57?focusedCommentId=92603&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-92603
Comment by Magnolia International [ 13/Oct/14 ]

...or the logic we have in Store there should be moved to the CachePolicy somehow ?

I tried that or just create voter for that but the problem is that we don't have the necessary information in AggregationState yet. All what we have is just URI.

I always prefer an explicit call to something like "is the node a variant, if so then we're uncacheable" rather than setting a header somewhere and as a side-effect we end up not caching the page.

I see your point. I wanted to get rid of p13n specific configuration at all because if you'd need a more specific store, you'd need to implement one for CE and one for EE.

Agreed on both point; the stuff I'm suggesting would require more changes in Cache module, I'm aware it's not possible as-is with the current API.

won't setting that header prevent browser caching too?

It does . I don't see it as a big problem as:

  1. dontCachePages voter is usually active anyway.
  2. Do we even want to do that? (it would be maybe good to cache a country variant but probably not a new/recurring visitor variant.

The problem is not so much the effect of the header itself than the fact that it's a hidden side-effect, and that the day it'll be a problem for someone somewhere, it'll be harder to track down that necessary.

Comment by Roman Kovařík [ 13/Oct/14 ]

Does it make sense to use MGNLCACHE-77 header instead?

Comment by Magnolia International [ 15/Oct/14 ]

I like the decoupling this permits between cache-Store and p13n. I don't like the "magic number" approach. Did you think of any possible alternatives ? (incl deeper changes to cache apis)

Comment by Roman Kovařík [ 24/Oct/14 ]

Reopen: adjust to changes in depending ticket.

Comment by Roman Kovařík [ 29/Oct/14 ]

Review by architects:

  1. HasVariantVoter needs javadoc
  2. If not, then when using Provider<AggState>, name it aggstateProvider (injected at different scope)
  3. PersonalizationIntegrationModuleVersionHandler : avoid bootstrap file for 2-properties node of isNotVariant, use NodeBuilder or simple tasks instead.
  4. HasVariantVoter is not-ed in config - maybe it’s easier to read if it’s positive ? WDYT about rename to IsCacheable (in the p13n packages) and avoid the “not” ?
  5. HasVariantVoter extends AbstractBoolVoter<Object> -> should be typed. This lead us to see that the passed value (CachedEntry) is actually not used by voter
Comment by Roman Kovařík [ 29/Oct/14 ]
  1. fixed.
  2. fixed.
  3. fixed.
  4. The idea was to make the voter more generic to make it possible to use elsewhere. We do the same in half of info.magnolia.voting.voters .
    Everything squashed on the branch MGNLPN-189-squash.
Generated at Mon Feb 12 06:35:11 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.