Last Comment Bug 956233 - enable USE_MEMCACHE on most objects
: enable USE_MEMCACHE on most objects
Status: RESOLVED FIXED
: bmo-goal
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 4.5.1
: All All
P1 enhancement (vote)
: Bugzilla 5.0
Assigned To: Byron Jones ‹:glob›
: default-qa
:
Mentors:
Depends on:
Blocks: 966180 975896
  Show dependency treegraph
 
Reported: 2014-01-02 22:34 PST by Byron Jones ‹:glob›
Modified: 2014-02-23 15:52 PST (History)
3 users (show)
glob: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
956233_1.patch (18.98 KB, patch)
2014-01-20 00:36 PST, Byron Jones ‹:glob›
dkl: review+
Details | Diff | Splinter Review

Description User image Byron Jones ‹:glob› 2014-01-02 22:34:30 PST
we should enable USE_MEMCACHE on most objects.
i'm aiming for all except bugs.
Comment 1 User image Frédéric Buclin 2014-01-03 02:57:49 PST
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 User image Byron Jones ‹:glob› 2014-01-06 18:39:10 PST
(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 User image Byron Jones ‹:glob› 2014-01-20 00:36:29 PST
Created attachment 8362399 [details] [diff] [review]
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 User image Byron Jones ‹:glob› 2014-01-23 01:00:06 PST
(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 User image David Lawrence [:dkl] 2014-01-29 12:35:24 PST
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 User image Byron Jones ‹:glob› 2014-01-30 23:16:55 PST
(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 User image Byron Jones ‹:glob› 2014-01-30 23:20:14 PST
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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.

Note You need to log in before you can comment on or make changes to this bug.