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

Make query cache rely on entity timestamp region #6001

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lcobucci
Member

lcobucci commented Sep 2, 2016

As far as I saw that's the default Hibernate behavior (https://vladmihalcea.com/2015/06/08/how-does-hibernate-query-cache-work/) to keep the query cache consistent.

I'm just not sure if that is the way to go, basically because of the change on the CachedEntityPersister interface (I didn't want to duplicate the logic that generates the timestamp key).

@lcobucci lcobucci force-pushed the lcobucci:fix/l2c-querycache branch from 5513384 to fac2097 Sep 2, 2016

@lcobucci

This comment has been minimized.

Member

lcobucci commented Sep 5, 2016

I had an idea to improve this PR a bit by moving the timestamp region check to the query cache validator but this also means moving the timestampKey to the QueryCacheKey.

I think it's a better solution, what's your opinion?

@guilhermeblanco

This comment has been minimized.

Member

guilhermeblanco commented Sep 6, 2016

@lcobucci indeed that's a MUCH BETTER solution. If I understood your idea correctly, you'd eliminate the BC break by adding a new interface method.
This means it could potentially be part of 2.X series instead of 3.X only. =)

If you're willing to work on that, I'd appreciate.

@lcobucci

This comment has been minimized.

Member

lcobucci commented Sep 6, 2016

@guilhermeblanco I really need this to be part of 2.X so I'm glad to help 😉

If I understood your idea correctly, you'd eliminate the BC break by adding a new interface method.

My idea basically is:

  1. Add timestampRegion to the TimestampQueryCacheValidator constructor
  2. Add timestampKey to the QueryCacheKey constructor
  3. Make TimestampQueryCacheValidator#isValid() also validate the timestamp using the region and the key

I couldn't see the BC break you said, can you please elaborate a bit more?

The biggest question here is: what'd be the best place to retrieve the timestampKey?
Passing it to the QueryCacheKey constructor means that we need to have it available also on the AbstractQuery#executeUsingQueryCache().

Moving those things to que validator also simplifies the AbstractEntityPersister#getHash() logic, since we don't need concatenate the $timestamp anymore (and we could also remove the unused timestampRegion from the persister).

/**
* @return \Doctrine\ORM\Cache\TimestampCacheKey
*/
public function getTimestampKey();

This comment has been minimized.

@Ocramius

Ocramius Sep 7, 2016

Member

bc break, sadly

This comment has been minimized.

@Ocramius

Ocramius Sep 7, 2016

Member

Also, I don't know how to regenerate a timestamp key, if not retrieved from here.

@lcobucci

This comment has been minimized.

Member

lcobucci commented Sep 7, 2016

Ow ofc. Do you think that the solution I explained is applicable?

@Ocramius

This comment has been minimized.

Member

Ocramius commented Sep 7, 2016

@lcobucci moving more info to the QueryCacheKey seems good to me, since it's information that sticks together anyway. I don't have the full picture though, sorry.

@lcobucci lcobucci force-pushed the lcobucci:fix/l2c-querycache branch from fac2097 to 2b98a59 Sep 7, 2016

@lcobucci

This comment has been minimized.

Member

lcobucci commented Sep 7, 2016

@guilhermeblanco @Ocramius introduced a tiny duplication in order to remove the BC break. I think it should be ok to be merged on v2.X now.

Anything else?

@lcobucci lcobucci force-pushed the lcobucci:fix/l2c-querycache branch from 2b98a59 to 7a80a90 Sep 7, 2016

private function regionUpdated(QueryCacheKey $key, QueryCacheEntry $entry)
{
if ($key->timestampKey === null) {

This comment has been minimized.

@Ocramius

Ocramius Sep 7, 2016

Member

The docblock states that it is never null: can you check that?

This comment has been minimized.

@lcobucci

lcobucci Sep 8, 2016

Member

The docblock is a liar! Will fix that, thanks

if ($key->lifetime == 0) {
return true;
}
return ($entry->time + $key->lifetime) > time();
}
private function regionUpdated(QueryCacheKey $key, QueryCacheEntry $entry)

This comment has been minimized.

@Ocramius

Ocramius Sep 7, 2016

Member

Docblock plz, even if it just says that a boolean is returned

This comment has been minimized.

@lcobucci

@lcobucci lcobucci force-pushed the lcobucci:fix/l2c-querycache branch from 7a80a90 to 5239722 Sep 8, 2016

@lcobucci

This comment has been minimized.

Member

lcobucci commented Sep 8, 2016

@guilhermeblanco @Ocramius comments processed let me know if there's anything else preventing this to be merged and let's :shipit:

@@ -38,18 +38,18 @@ class QueryCacheEntry implements CacheEntry
/**
* READ-ONLY: Public only for performance reasons, it should be considered immutable.
*
* @var integer Time creation of this cache entry
* @var float Time creation of this cache entry

This comment has been minimized.

@Ocramius

Ocramius Sep 8, 2016

Member

Since there was a change here, did you check all usages of this property?

This comment has been minimized.

@lcobucci

lcobucci Sep 8, 2016

Member

I did but I definitely forgot to change ($entry->time + $key->lifetime) > time() to ((int) $entry->time + $key->lifetime) > time() on the validator

* @param string $hash Result cache id
* @param integer $lifetime Query lifetime
* @param int $cacheMode Query cache mode
* @param TimestampCacheKey|null $timestampKey

This comment has been minimized.

@Ocramius

Ocramius Sep 8, 2016

Member

When is null happening here, exactly?

This comment has been minimized.

@lcobucci

lcobucci Sep 8, 2016

Member

Basically because of

public function testNonCacheableQueryDeleteStatementException()
and
public function testNonCacheableQueryUpdateStatementException()

Since the ResultSetMapping don't give the entity name.

$this->assertCount(3, $result2);
$this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());

This comment has been minimized.

@Ocramius

Ocramius Sep 8, 2016

Member

Wait... Shouldn't this be $queryCount + 2?

This comment has been minimized.

@Ocramius

Ocramius Sep 8, 2016

Member

I would expect that because of the region being updated (and therefore cache evicted)

This comment has been minimized.

@Ocramius

Ocramius Sep 8, 2016

Member

Argh, totally misread, sorry. Missed that you are resetting the $queryCount after the first query.

lcobucci added some commits Sep 2, 2016

The timestamp verification is now done by the validator
So it's useless to keep it here too.

@lcobucci lcobucci force-pushed the lcobucci:fix/l2c-querycache branch from 5239722 to 9130d8c Sep 8, 2016

Ocramius added a commit that referenced this pull request Sep 8, 2016

Ocramius added a commit that referenced this pull request Sep 8, 2016

Ocramius added a commit that referenced this pull request Sep 8, 2016

@Ocramius Ocramius self-assigned this Sep 8, 2016

@Ocramius Ocramius added this to the 2.5.5 milestone Sep 8, 2016

Ocramius added a commit that referenced this pull request Sep 8, 2016

@Ocramius Ocramius closed this in 009e947 Sep 8, 2016

@Ocramius

This comment has been minimized.

Member

Ocramius commented Sep 8, 2016

This goes into 2.5.5! Thanks @lcobucci

master: 009e947
2.5: e16de70

@lcobucci lcobucci deleted the lcobucci:fix/l2c-querycache branch Sep 8, 2016

yvoyer added a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

alexgurrola added a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017

alexgurrola added a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment