Loading multiple taxonomy terms ConsistentCacheBase::matchKey keeps returning the same values for each entity [#2533274]

Comment	File	Size	Author
#2	loading_multiple-2533274-1.patch	693 bytes	joelpittet
	cache_consistent_inc_-_html.png	150.13 KB	joelpittet

Comment #1

joelpittet

he/him

English

Vancouver

commented 14 July 2015 at 23:13

Yeah from the screenshot this evaluates to TRUE. I think both sides need to be cast to strings. I'll give that a try and post a patch but just some rough though noting here:

php -r 'var_dump(strpos(1960, 1329) === 0);'

http://3v4l.org/fG06S

Log in or register to post comments

Comment #2

joelpittet

he/him

English

Vancouver

commented 14 July 2015 at 23:20

Status:

Active

» Needs review

Status	File	Size
new	loading_multiple-2533274-1.patch	693 bytes

This resolved it for me and should be a tiny bit faster if they are exact matches.

Log in or register to post comments

Comment #3

gielfeldt commented 15 July 2015 at 01:10

Thanks. Nice catch. I wonder if this is actually what caused the problem for you with ConsistentCacheLookup as well. It seems to suffer from the same issue, although the symptom was a bit different.

Log in or register to post comments

Comment #6

gielfeldt commented 15 July 2015 at 06:24

I've added your fix, and I've also added the fix to the CacheConsistentLookup class. I've also added test-cases for the issue.

Please let me know if it works. And if possible, please let me know if the CacheConsistentLookup is now fixed wrt #2530334: Notice: Undefined offset: 92 in ConsistentCacheBuffer->lookup() (line 179 of cache_consistent/cache_consistent.lookup.inc).

Log in or register to post comments

Comment #7

joelpittet

he/him

English

Vancouver

commented 15 July 2015 at 15:58

Thanks @gielfeldt, not sure which one I should stick with but for now it's not the lookup. The lookup did sound better for me with this commerce + redis site but I don't know...

Log in or register to post comments

Comment #8

gielfeldt commented 16 July 2015 at 16:35

Ok. Just let me know if the regular one works, if possible. The lookup version is probably only better on paper anyways :-)

Log in or register to post comments

Comment #9

joelpittet

he/him

English

Vancouver

commented 16 July 2015 at 16:39

Well I have one weird problem with cache loosing property info for commerce_line_item bundles but I don't think it's related to this class (yet...).

So far 24 hours of goodness.

:)

Log in or register to post comments

Comment #10

joelpittet

he/him

English

Vancouver

commented 17 July 2015 at 22:33

Ran into a bunch of these in my local logs just now:

Warning: strpos(): Empty needle in ConsistentCacheBase->matchKey() (line 180 of cache_consistent/cache_consistent.inc).

Should that be possible? And if so, then I guess we need to check $key !== ''?

Log in or register to post comments

Comment #11

gielfeldt commented 17 July 2015 at 23:59

Darn it. It is possible, if someone calls cache_set or cache_clear_all with an "" for cid. Which I have encountered before. And by looking at the code, the only way I can see that this warning should occur, is if indeed someone calls cache_set or cache_clear_all with an "" for cid. But you never know ...

If you're on a local setup, could you perhaps check do some debugging in line 260 and 292, and check if someone is actually performing cache-operations with empty string cache ids?

Log in or register to post comments

Comment #12

gielfeldt commented 18 July 2015 at 06:25

Wait a minute ... cache_clear_all() accepts an empty $cid, of course. I think the check you propose might suffice as a fix. I'll get back to you.

Log in or register to post comments

Comment #13

gielfeldt commented 23 July 2015 at 15:53

Looks like I need some extra logic for handling cache flushes...

Log in or register to post comments

Comment #14

joelpittet

he/him

English

Vancouver

commented 23 July 2015 at 19:03

Yay, extra logics!

Log in or register to post comments

Comment #15

gielfeldt commented 24 July 2015 at 06:06

Holy mother of God, I may have to refactor quite a bit :-(

Log in or register to post comments

Comment #16

gielfeldt commented 25 July 2015 at 06:10

I've fixed some bugs regarding flushes and minimum cache lifetime. I've also added more tests, that should cover the issues we've seen. Could you try out the latest dev-version?

After the refactoring the buffering logic became a bit more complex, making me consider I might phase out the regular class, so that it's only the lookup version that remains.

Also, the syntax of the configuration has changed:

$conf['cache_default_class'] = 'ConsistentCache';
$conf['consistent_cache_default_buffer_mechanism'] = 'ConsistentCacheBufferAdvanced';

If I remove the regular buffering class, the default buffer class will become "ConsistentCacheBufferAdvanced", so that line 2 above will be unnecessary.

If it works, I may also refactor some of the naming ("cache consistent" vs "consistent cache") before a next release candidate.

Log in or register to post comments

Comment #17

gielfeldt commented 28 July 2015 at 13:47

Status:

Needs review

» Postponed (maintainer needs more info)

Log in or register to post comments

Comment #18

joelpittet

he/him

English

Vancouver

commented 28 July 2015 at 14:27

Why was the configuration needing to change?

I'll see if I can give this a try today but a bit leary to put large changes in production with commerce without having at least another person testing(besides you who wrote the fixes).

Log in or register to post comments

Comment #19

joelpittet

he/him

English

Vancouver

commented 28 July 2015 at 16:58

you may want to put php = 5.3 in the info file because I noticed you were using __DIR__ magic constant and it was introduced then.

Log in or register to post comments

Comment #20

gielfeldt commented 28 July 2015 at 17:14

Thx for the heads up. I might change it to dirname(__FILE__) instead though.

The configuration didn't need to change as such, but I generalised the buffer classes, so that I could easier maintain them. In any case, I will most likely remove the simple class, so the configuration (when I'm done) will just be:

$conf['cache_default_class'] = 'ConsistentCache';

Also, currently in the dev-version, using:

$conf['cache_default_class'] = 'ConsistentCacheLookup';

is equivalent to:

$conf['cache_default_class'] = 'ConsistentCache';
$conf['consistent_cache_default_buffer_mechanism'] = 'ConsistentCacheBufferAdvanced';

But that will change.

Log in or register to post comments

Comment #21

joelpittet

he/him

English

Vancouver

commented 28 July 2015 at 17:25

Well, just updating to latest -dev this seemed to fix an issue where some menu items went missing... correlation maybe but that was nice to see:)

Should $conf['cache_default_class'] = 'ConsistentCache'; still work with the latest -dev if I don't want to use the lookup?

Log in or register to post comments

Comment #22

gielfeldt commented 28 July 2015 at 17:34

Yes, that should still work.

Log in or register to post comments

Comment #23

gielfeldt commented 28 July 2015 at 17:43

wrt menus, was the problem you had somewhat similar to this one? #2469925: Child menu items disappear

Log in or register to post comments

Comment #24

joelpittet

he/him

English

Vancouver

commented 28 July 2015 at 17:53

Yes I think that is the problem it sounds very very similar.

Log in or register to post comments

Comment #25

gielfeldt commented 31 July 2015 at 08:51

I've merged my refactoring into 7.x-2.x-dev.

I've removed the Lookup class. You now only need to:

<?php
$conf['cache_default_class'] = 'ConsistentCache';
?>

Log in or register to post comments

Comment #26

joelpittet

he/him

English

Vancouver

commented 31 July 2015 at 17:02

Thanks, I'll upgrade.

Log in or register to post comments

Comment #27

joelpittet

he/him

English

Vancouver

commented 1 August 2015 at 20:49

Status:

Postponed (maintainer needs more info)

» Needs work

Noticed this in my logs regarding a elysia cron job. It may not be a problem here but should probably be checked into.

Notice: Undefined offset: 76 in ConsistentCacheBuffer->lookup() (line 280 of cache_consistent/cache_consistent.buffer.inc).

Log in or register to post comments

Comment #28

gielfeldt commented 2 August 2015 at 13:14

I have a hard time reproducing this. But I have a theory. Could you try to remove or comment line 327 in cache_consistent.buffer.inc ? And see if that eliminates the warning.

Log in or register to post comments

Comment #29

gielfeldt commented 2 August 2015 at 13:34

Nevermind. I found the bug.

Log in or register to post comments

Comment #30

gielfeldt commented 2 August 2015 at 14:05

There. Could you try the latest dev version? Again :-)

Log in or register to post comments

Comment #31

joelpittet

he/him

English

Vancouver

commented 4 August 2015 at 00:37

Thanks, I'll give it a try.

Should I worry about this change?

   public function prune() {
-    foreach ($this->lookupTable as $cid => &$operations) {
-      if ($operations) {
-        $only = array_pop($operations);
-        foreach ($operations as $idx => $operation) {
-          unset($this->buffer[$idx]);
-        }
-        $operations = array($only => $only);
-      }
-    }
+    // @todo Make this work.
   }

Log in or register to post comments

Comment #32

gielfeldt commented 4 August 2015 at 07:02

No, it was just a performance optimisation that wasn't working as well as it should. So I removed it. I've since then re-implemented it so that it works better, but I haven't pushed it to the dev-branch yet.

Log in or register to post comments

Comment #33

joelpittet

he/him

English

Vancouver

commented 5 August 2015 at 01:07

~~So it seems to be throwing notices, but to the end user on admin_menu cache clears it's showing like an exception error.~~

Notice: Undefined offset: 42 in ConsistentCacheBuffer->lookup() (line 280 of cache_consistent/cache_consistent.buffer.inc)

~~I noticed no new changes have been made since yesterday when I applied that so I assume this is from the new changes.~~

Edit: before patch applied.

Log in or register to post comments

Comment #34

joelpittet

he/him

English

Vancouver

commented 4 August 2015 at 21:46

Whoops, hold up didn't push my changes live yet. Just did that... ignore #33

Log in or register to post comments

Comment #35

gielfeldt commented 6 August 2015 at 20:03

No news is good news, I hope? Since last time, I've fixed a couple of edge-cases, which you probably haven't run into. I'm gonna merge them into 2.x-dev tonight.