Multi Get support for Second Level Cache #954

goetas · 2014-02-14T13:48:06Z

Hi!
As discussed here i have implemented some kind of multi-get for second level cache implementation.

I have also created a PR to doctrine/cache component (see doctrine/cache#29) that enables this feature at driver cache level.

doctrinebot · 2014-02-14T13:48:10Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2982

We use Jira to track the state of pull requests and the versions they got
included in.

goetas · 2014-02-14T14:23:16Z

I have not added tests yet because i think that it has to be discussed...

FabioBatSilva · 2014-02-14T15:41:53Z

I'd like to suggest Doctrine\ORM\Cache\Region\DefaultMultiGetRegion and Doctrine\ORM\Cache\MultiGetCollectionHydrator

That will remove the instanceof logic from DefaultCollectionHydrator and keep DefaultGetRegion simple..

goetas · 2014-02-14T15:43:55Z

@FabioBatSilva ok, but then have i to edit DefaultCacheFactory::getRegion() wrapping a default impl?

goetas · 2014-02-14T15:49:59Z

@FabioBatSilva
Question: i have seen that you have used everywhere Doctrine\Common\Cache\CacheProvider as dependency from docrine/cache...
Why not use just Doctrine\Common\Cache\Cache? It is an interface... (deps on interfaces are better...)

goetas · 2014-02-14T15:54:28Z

@FabioBatSilva i do not understand how to implement DefaultCollectionHydrator... i have to check aniway if $targetRegion is instanceof MultiGetRegion...

FabioBatSilva · 2014-02-14T16:38:23Z

It depends on CacheProvider because cache regions use flushAll which is not part of Cache
But in reality only concrete implementations know about Common\Cache, you could have a different implementations that uses wherever cache provider you want..

For the MultiGetCollectionHydrator I think u need to wrap DefaultCacheFactory#buildCollectionHydrator and check the region to decide which implementation will be used..

goetas · 2014-02-17T07:48:20Z

I'm not sure to use inheritance over composition inside DefaultMultiGetRegion...

skydiablo · 2014-06-03T10:17:38Z

some proceedings ?

goetas · 2014-06-14T18:19:30Z

at the moment i'm very busy... :-(

Ocramius · 2015-01-15T22:41:44Z

@goetas since doctrine/cache#29 is fixed, could you eventually complete this PR next week, so that we can throw it in the final 2.5 release? (probably doing it while at PHPBNL15)

goetas · 2015-01-16T07:56:23Z

@Ocramius i can to look at this this evening or on tomorrow....!

goetas · 2015-01-16T23:19:54Z

lib/Doctrine/ORM/Cache/DefaultCacheFactory.php

+        if ($mapping['cache']) {
+            $targetPersister = $em->getUnitOfWork()->getEntityPersister($mapping['targetEntity']);
+
+            if ($targetPersister instanceof CachedPersister) {


I really hate this nested if... but I do not know why $em->getUnitOfWork()->getEntityPersister($mapping['targetEntity']); somtimes seems to be instance of BasicEntityPersister.... and not a cache persister as expected...

Ping @guilhermeblanco about this

found that one!
I don not know why but Doctrine\Tests\Models\CMS\CmsUser::$articles is marked as cacheable (but CmsArticle is not!)... and this is the only failing test case

goetas · 2015-01-16T23:20:56Z

@Ocramius rebased and tested (a bit)

Ocramius · 2015-01-16T23:25:35Z

Thanks @goetas. I'm stuck on another PR and the current build failure, but I'll get to this one tonight, hopefully.

Ocramius · 2015-01-17T07:53:08Z

lib/Doctrine/ORM/Cache/MultiGetRegion.php

+ * @since   2.5
+ * @author  Asmir Mustafic
+ */
+interface MultiGetRegion extends Region


Don't extend from larger interfaces unless strictly needed

Does it make sense? A MultiGetRegion region is always a region. Of course, the new interface is much simpler, but can it be used alone? I can not imagine a use case.

Ocramius · 2015-01-17T07:53:40Z

This PR should introduce a dependency to "doctrine/cache": "~1.4"

Ocramius · 2015-01-17T07:55:49Z

lib/Doctrine/ORM/Cache/DefaultCacheFactory.php

+                $targetRegion = $targetPersister->getCacheRegion();
+
+                if ($targetRegion instanceof MultiGetRegion) {
+                    return new MultiGetCollectionHydrator($em);


The multi-get region is not being passed to the MultiGetCollectionHydrator? You are doing a lot of checks but discarding a lot of dependencies then: is it intended to be like that?

With the first version I was re-building the multi-get region on each loadCacheEntry :-( , while now I pass it as a constructor argument.

Your suggestion makes it simpler and faster, thanks! :-)

I have "fixed" this problem with goetas@e5ee538 but i think that is not the right solution...

@Ocramius what about this?

guilhermeblanco · 2015-01-17T16:14:30Z

lib/Doctrine/ORM/Cache/DefaultCacheFactory.php

@@ -202,7 +218,11 @@ public function getRegion(array $cache)

        $cacheAdapter->setNamespace($cache['region']);

-        $region = new DefaultRegion($cache['region'], $cacheAdapter, $this->regionsConfig->getLifetime($cache['region']));
+        if ($cacheAdapter instanceof MultiGetCache) {


$region = ($cacheAdapter instanceof MultiGetCache) ? new DefaultMultiGetRegion($cache['region'], $cacheAdapter, $this->regionsConfig->getLifetime($cache['region'])) : new DefaultRegion($cache['region'], $cacheAdapter, $this->regionsConfig->getLifetime($cache['region']));

goetas · 2015-01-17T17:45:11Z

@guilhermeblanco I've tried to implement buildCacheEntry in the way as suggested by you

…ersioned associations

multiget region

goetas · 2015-01-17T19:51:17Z

@guilhermeblanco solved the [bug](goetas@0b57935#diff-408359faa7db56dc64cf1f0b2006625dL8%5D! :-)

Versioned classes can't be cacheable as associations....

/cc @Ocramius

…eed to segregate the interface here)

…neric cache adapter

…-get cache regions

…Common\Cache\Cache` instances

…ultMultiGetRegion` instantiation logic

Ocramius · 2015-01-17T22:54:59Z

Merged after applying following changes:

DefaultRegionFactory now accepts generic Doctrine\Common\Cache\Cache
DefaultRegionFactory builds a MultiGetRegion when the given cache adapter is a MultiGetCache
DefaultRegion now operates just with a Doctrine\Common\Cache\Cache, evictAll will throw an exception if the cache instance is not a FlushableCache
DefaultRegionFactory sets the cache namespace only if a CacheProvider is given
Region now extends MultiGetRegion (breaking change, but we didn't yet release SLC, so it's fine)

The SRP violation of Region being a child of MultiGetRegion is negligible, IMO, as it is trivial to provide a generic polyfill for the getMultiple implementation

master: 4cde35d

Ocramius · 2015-01-17T22:55:04Z

Thanks @goetas!

Ocramius self-assigned this Jan 15, 2015

goetas force-pushed the multiget branch from 5be794e to 407d203 Compare January 16, 2015 23:16

goetas reviewed Jan 16, 2015
View reviewed changes

Ocramius reviewed Jan 17, 2015
View reviewed changes

goetas added 3 commits January 17, 2015 10:13

Added multi-get support for second level cached collections

08d5e70

Added test case for MultiGetRegion

7748716

Added test case for buildCachedCollectioHydrator on DefaultCacheFactory

cb9e1de

goetas force-pushed the multiget branch from 407d203 to a8f871c Compare January 17, 2015 09:15

goetas added 3 commits January 17, 2015 10:29

Added dependency with doctrine/cache 1.4

10331a5

Removed dependency with Region interface

4c3fbee

MultiGetCollectionHydrator depends knows the multi-get region

c131e32

goetas force-pushed the multiget branch from 01e3604 to c131e32 Compare January 17, 2015 09:29

guilhermeblanco reviewed Jan 17, 2015
View reviewed changes

goetas force-pushed the multiget branch from 7298209 to cc47d6a Compare January 17, 2015 18:24

goetas added 3 commits January 17, 2015 20:48

Test suite: Second level cache listener should not mark "cacheable" v…

0b57935

…ersioned associations

New buildCacheEntry way bo build a entry for a cached collection

b90576c

Changed some tests to be compatible with the new implementation of

7494d2d

multiget region

goetas force-pushed the multiget branch from cc47d6a to 7494d2d Compare January 17, 2015 19:48

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - Making cache Region always a MultiGetRegion (no n…

3c5a794

…eed to segregate the interface here)

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - No need to check if a Region is a MultiGetRegion

8ddcc4b

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - Minor CS fixes/IDE hints

95c6cca

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - Evicting all cache entries is not supported with a ge…

0e4a7ca

…neric cache adapter

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - Evicting all cache entries is not supported with a ge…

5646248

…neric cache adapter

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - Coverage for different instantiation of single-/multi…

95fe03b

…-get cache regions

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - DefaultCacheFactory now supports generic `Doctrine\…

624b985

…Common\Cache\Cache` instances

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - Better type-safety in `Doctrine\ORM\Cache\Region\Defa…

b147476

…ultMultiGetRegion` instantiation logic

Ocramius added a commit that referenced this pull request Jan 17, 2015

#954 DDC-2982 - s/CacheProvider/Cache in documentation

d5f6b44

Ocramius closed this in 4cde35d Jan 17, 2015

goetas deleted the multiget branch January 18, 2015 08:20

goetas mentioned this pull request May 5, 2015

Fix issue with second-level-cache tests and versioned entities #1380

Closed

doctrinebot mentioned this pull request Dec 6, 2015

DDC-2982: [GH-954] Multi Get support for Second Level Cache #3746

Closed

orfin mentioned this pull request Jun 20, 2016

Second Cache Level - allow "default_multi" in region type doctrine/DoctrineBundle#549

Closed

polyanin mentioned this pull request Jul 13, 2016

Multi get implementation in QueryCache #5831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi Get support for Second Level Cache #954

Multi Get support for Second Level Cache #954

goetas commented Feb 14, 2014

doctrinebot commented Feb 14, 2014

goetas commented Feb 14, 2014

FabioBatSilva commented Feb 14, 2014

goetas commented Feb 14, 2014

goetas commented Feb 14, 2014

goetas commented Feb 14, 2014

FabioBatSilva commented Feb 14, 2014

goetas commented Feb 17, 2014

skydiablo commented Jun 3, 2014

goetas commented Jun 14, 2014

Ocramius commented Jan 15, 2015

goetas commented Jan 16, 2015

goetas Jan 16, 2015

Ocramius Jan 17, 2015

goetas Jan 17, 2015

goetas commented Jan 16, 2015

Ocramius commented Jan 16, 2015

Ocramius Jan 17, 2015

goetas Jan 17, 2015

Ocramius commented Jan 17, 2015

Ocramius Jan 17, 2015

goetas Jan 17, 2015

goetas Jan 17, 2015

goetas Jan 17, 2015

guilhermeblanco Jan 17, 2015

goetas commented Jan 17, 2015

goetas commented Jan 17, 2015

Ocramius commented Jan 17, 2015

Ocramius commented Jan 17, 2015

Multi Get support for Second Level Cache #954

Multi Get support for Second Level Cache #954

Conversation

goetas commented Feb 14, 2014

doctrinebot commented Feb 14, 2014

goetas commented Feb 14, 2014

FabioBatSilva commented Feb 14, 2014

goetas commented Feb 14, 2014

goetas commented Feb 14, 2014

goetas commented Feb 14, 2014

FabioBatSilva commented Feb 14, 2014

goetas commented Feb 17, 2014

skydiablo commented Jun 3, 2014

goetas commented Jun 14, 2014

Ocramius commented Jan 15, 2015

goetas commented Jan 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Jan 16, 2015

Ocramius commented Jan 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Jan 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Jan 17, 2015

goetas commented Jan 17, 2015

Ocramius commented Jan 17, 2015

Ocramius commented Jan 17, 2015