956233 - enable USE_MEMCACHE on most objects

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[:glob ✱]

we should enable USE_MEMCACHE on most objects.
i'm aiming for all except bugs.

Comment 1

[Frédéric Buclin]

Once this is turned on, does it still make sense to keep this constant? If an admin doesn't want to use memcached, he is free to not set the memcached_servers parameter, which has the same effect as disabling memcached.

Comment 2

[:glob ✱]

(In reply to Frédéric Buclin from comment #1)
> Once this is turned on, does it still make sense to keep this constant? If
> an admin doesn't want to use memcached, he is free to not set the
> memcached_servers parameter, which has the same effect as disabling
> memcached.

leaving it in would allow for extensions who hit the database directly in bad ways to quickly opt-out of the cache.  i can't decide if this is a good idea or not :)

Comment 3

[:glob ✱]

Attachment: 956233_1.patch

this enables memcached by default on most objects except bugs.

the basic rule is any time we directly touch a table which has a bugzilla::object class, we need to clear that entry from memcached.

Comment 4

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #2)
> leaving it in would allow for extensions who hit the database directly in
> bad ways to quickly opt-out of the cache.  i can't decide if this is a good
> idea or not :)

while developing the patch i hit a few objects where there was no benefit in using memcached due to how they are used within bugzilla.  we'll need to leave the const in to allow per-class opt-out where appropriate.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 8362399 [details] [diff] [review]
956233_1.patch

Review of attachment 8362399 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine and works well. The column error can be fixed on checking. r=dkl

::: Bugzilla/Classification.pm
@@ +65,5 @@
> +            $dbh->sql_in('id', \@product_ids));
> +        foreach my $id (@product_ids) {
> +            Bugzilla->memcached->clear({ table => 'products', id => $id });
> +        }
> +    }

techincally the UI will not allow a classification to be deleted if one or more products are assigned to it but this is good to have anyway.

::: Bugzilla/Field.pm
@@ +1075,5 @@
>          # Restore the original obsolete state of the custom field.
>          $dbh->do('UPDATE fielddefs SET obsolete = 0 WHERE id = ?', undef, $field->id)
>            unless $is_obsolete;
> +
> +        Bugzilla->memcached->clear({ table => 'fielddefs', id => $field->id });

I don't see where we are actually storing the field objects in the memcache but leave this here is harmless for now.

I suspect what is happening (and in several other places) is that since we do not cache objects loaded by match() and/or new_from_list() that certain objects do not get stored. Then when we delete them from the cache individually, they are not found. Either way this is harmless IMO and when we do get the above methods working with the cache, then the clear() parts will already be there.

::: Bugzilla/FlagType.pm
@@ +186,5 @@
>      # specifically requestable.
>      if (!$self->is_requesteeble) {
> +        my @ids = $dbh->selectrow_array(
> +            "SELECT id FROM flags WHERE type_id = ?", undef, $self->id);
> +        $dbh->do("UPDATE flags SET requireee_id = NULL WHERE "

DBD::mysql::db do failed: Unknown column 'requireee_id' in 'field list'

s/requiree_id/requestee_id/

Comment 6

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #5)
> techincally the UI will not allow a classification to be deleted if one or
> more products are assigned to it but this is good to have anyway.
> ...
> I don't see where we are actually storing the field objects in the memcache
> but leave this here is harmless for now.

don't forget that extensions may be using objects in ways that are different from how the core code operates, so support for these operations is required.

Comment 7

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified buglist.cgi
modified editclassifications.cgi
modified editusers.cgi
modified query.cgi
modified sanitycheck.cgi
modified userprefs.cgi
modified Bugzilla/Attachment.pm
modified Bugzilla/Bug.pm
modified Bugzilla/Classification.pm
modified Bugzilla/Field.pm
modified Bugzilla/Flag.pm
modified Bugzilla/FlagType.pm
modified Bugzilla/Memcached.pm
modified Bugzilla/Milestone.pm
modified Bugzilla/Object.pm
modified Bugzilla/User.pm
modified Bugzilla/Auth/Verify.pm
modified Bugzilla/Auth/Verify/DB.pm
modified Bugzilla/Comment/TagWeights.pm
modified Bugzilla/Install/Requirements.pm
modified Bugzilla/Search/Recent.pm
modified Bugzilla/Search/Saved.pm
modified contrib/merge-users.pl
Committed revision 8906.