[MGNLCACHE-57] Avoid unnecessary cache locking for uncacheable entries Created: 16/May/14  Updated: 03/Feb/16  Resolved: 29/Oct/14

Status: Closed
Project: Cache Modules
Component/s: None
Affects Version/s: None
Fix Version/s: 5.3

Type: Improvement Priority: Major
Reporter: Magnolia International Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
dependency
is depended upon by MGNLADVCACHE-29 EagerRecacheCachePolicy doesn't take ... Closed
is depended upon by MGNLPN-189 Remove personalisation-specific cache... Closed
relation
is related to MGNLCACHE-78 Move store caching configuration into... Closed
supersession
supersedes MGNLPN-112 Bypass cache for personalized pages Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Release notes required:
Yes
Date of First Response:

 Description   

Currently, "header negotiation" lets page components set Cache* headers, which in turns sets the TTL of a cache entry to 0. The Store CacheExecutor then sees this, "replays" the cache entry immediately and discards it (rather than putting it in the cache and letting the useCache executor handle it)

While implementing MGNLPN-112, we realized that we could instead store a "marker" entry in the cache, and on further access to such entry, the cache policy could simply instruct the cache to be bypassed.



 Comments   
Comment by Magnolia International [ 16/May/14 ]

Revise MGNLPN-112 when this gets implemented (e.g for personalization, only the BypassVariantsCacheStore will be needed, BypassUncacheableEntriesPolicy could be merged with i.m.m.c.cachepolicy.Default and UncacheableEntry moved to the cache module as well.

Comment by Magnolia International [ 25/Jul/14 ]

TODO: evaluate: hit-rates can be messed up this feature as implemented in MGNLPN-112. (lots of "hits" recorded for pages that are actually not in the cache) Instead, we could maybe use another cache or a simple in-memory Set) for {{UncacheableEntry}}s.

Comment by Roman Kovařík [ 08/Oct/14 ]
  1. Merged solution from MGNLPN-122
  2. Entry with ttl=0 is saved as UncachableEnry instead of discarded.
  3. Another cache neither in-memory set was not used since hit-rates are messed up by the way how we e.g. check for item existence anyway.
Comment by Magnolia International [ 09/Oct/14 ]

Looks good. Couple of comments on UncacheableEntry:

  • both throw new IllegalStateException("Should not be used”)
    • should be commented (why)
    • throw exception with a better message
  • the synchronize() block in replay() — won’t that lock all concurrent requests to this entry ?
    • if so, we might have a problem (i think I originally copied the blockfrom somewhere else but I can't recall where/why ??)
    • either way, I'd recommend also adding a comment to explain it

Good call on making MGNLPN-189; see other comments over there.

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

both throw new IllegalStateException("Should not be used”) should be commented (why):

I realised that we actually need these methods when fixing MGNLADVCACHE-29.

the synchronize() block in replay() — won’t that lock all concurrent requests to this entry

Please see the last commit.

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

won't setting that header prevent browser caching too?

Should we introduce e.g. X-Magnolia-Cache header to be able use it independently from browser caching?

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

rename 'isUncacheableVoters' to positive 'isCacheableVoters'

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

Review by architects:

  1. We’re not 100% convinced by isCacheableVoters’s default “AND” op, and the needed for checking getVoters().length == 0 - i.e we see why you had to do it, but this is yet another thing we don’t like about the whole voting API.
  2. The commit to CacheModuleVersionHandler is “full" of codestyle changes. Please avoid.
  3. maybe these Voters could vote on AggregationState instead of CacheEntry?
Comment by Roman Kovařík [ 29/Oct/14 ]
  1. Whole empty default configuration for isCacheableVoters’s was removed so there is no need to check getVoters().length == 0 anymore.
  2. Fixed and all merged on the branch MGNLCACHE-57-merge.
  3. I thing in general is better to have CacheEntry there since it contains a lot of informations. I think that p13n is the specific case where we need something else
Comment by Roman Kovařík [ 06/Nov/14 ]

isCacheableVoters moved back to Store

Generated at Sun Feb 11 23:51:47 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.