enable USE_MEMCACHE on most objects

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Bugzilla-General
P1
enhancement
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

({bmo-goal})

4.5.1
Bugzilla 5.0
bmo-goal
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

3 years ago
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.
Severity: normal → enhancement
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → 4.5.1

Updated

3 years ago
Priority: -- → P1
(Assignee)

Comment 2

3 years ago
(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 :)
(Assignee)

Comment 3

3 years ago
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.
Attachment #8362399 - Flags: review?(dkl)
(Assignee)

Comment 4

3 years ago
(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.

Updated

3 years ago
Status: NEW → ASSIGNED
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/
Attachment #8362399 - Flags: review?(dkl) → review+
(Assignee)

Comment 6

3 years ago
(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.
Flags: approval+
(Assignee)

Updated

3 years ago
Blocks: 966180
(Assignee)

Comment 7

3 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Target Milestone: --- → Bugzilla 5.0

Updated

3 years ago
Blocks: 975896
You need to log in before you can comment on or make changes to this bug.