-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
CRM-21109 Add test & consolidate code controlling cache clearing, specific improvement on cli imports #10943
Conversation
I think it's backwards that we write cache entries on contact.create rather than on read operations, and that if we reversed that we wouldn't need to tackle this sort of thing. But that's a bigger story than this. For the above, I'd feel more comfortable if it said API calls could defer cache clearing until after a set of operations was completed - rather than just switch it off, which feels more likely to land in an inconsistent state. If the API has any command over cache at all (🤢) then perhaps it could do so in a way that has less side-effects, eg "pause cache updates; write 100 contacts; unpause". |
OK based on further discussion with @xurizaemon here is my proposal
|
If we don't think an api for turning the cache status on & off is OK we could just have it as a function - but then we are saying 'hey external script people, call this, but it's not really supported' |
d0f0e3e
to
e78d0c1
Compare
@MegaphoneJon I'd like to propose this as an alternate fix to your PR. It does SOME of what I have suggested above - specifically
I am inclined to add a setting 'cache_refresh_mode' & use that rather than doNotResetCache - but think it makes sense as a follow up. This PR as it sits is IMHO cleanup + fix rather than a real change |
e78d0c1
to
a5fdac1
Compare
Random reactions -- no particular order:
I like the idea of consolidating on a setting. The For non-PHP consumers... how would you map into APIv3? Perhaps civicrm_api3('Contact', 'create', array(
'options' => array('settings' => array('cache_refresh_mode' => 'deterministic')),
'first_name' => '...',
...,
)); I like this from a flexibility+consistency perspective. However, you'd need some kind of permissioning -- not every API consumer should be trusted with access to every setting. But where to do draw the line... |
a5fdac1
to
71a229e
Compare
At this stage I have done
and it seems there is enough agreement about
for me to do that. I'm inclined to punt the api function for now. Note two existing related api functions exist
|
154d0e0
to
d985db2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton setPermitCacheFlushMode
is being set TRUE when I think it should be FALSE and vice versa - EXCEPT in the new instance (class.cli.php
), which looks right. However, with the docblock missing, it's hard to say. Could you please look those calls over and ensure they're correct?
CRM/Core/Config.php
Outdated
/** | ||
* Is the system permitted to flush caches at the moment. | ||
* | ||
* This can be turned off by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two docblocks are incomplete.
9c9544d
to
8bdcaf8
Compare
@MegaphoneJon you are right - I changed from double negative somewhere along the way to positive & didn't correct it properly. Jenkins also picked it up. Hopefully fixed now - Jenkins is passing opinion as we speak |
Womp womp, 5 failures :( |
f815f53
to
07d8e57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the changes @eileenmcnaughton made; the remaining issues are in the test suite. They should be fixed, but then I'd say this should merge-on-pass.
public function testCreateIndividualNoCacheClear() { | ||
|
||
$contact = $this->callAPISuccess('contact', 'create', $this->_params); | ||
$groupID = $this->groupCreate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't getting cleaned up in tearDown()
which causes the next test to use this method to fail because the group already exists.
0aa73ca
to
0edf9c7
Compare
0edf9c7
to
1157c81
Compare
@colemanw @MegaphoneJon got there - last changes were to caching & tests because I made CRM_Contact_BAO_GroupContactCache::remove() protected & had to adjust tests to that. If anyone has any enthusiasm to review a follow up about half of the remove() function can go now as it is no longer being called without $groupID being passed in anymore (yay) |
f82205d
to
a767bc1
Compare
test this please |
@@ -678,7 +588,8 @@ public static function load(&$group, $force = FALSE) { | |||
AND civicrm_group_contact.group_id = $groupID "; | |||
|
|||
$groupIDs = array($groupID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can now be moved down a few lines (to above self::updateCacheTime($groupIDs, $processed);
), which I think would make it easier to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MegaphoneJon thanks for taking a look!. I fixed the issue you identified & also tried to make the array vs int clearer in a few places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! If you push that commit into this PR I'll take a look.
|
||
CRM_Core_DAO::executeQuery($query, $params); | ||
if ($refresh) { | ||
CRM_Core_DAO::executeQuery($refresh, $params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant since we're deleting it - but this query will always be overwritten by the $update
query. Oops.
@@ -217,7 +217,7 @@ public static function loadAll($groupIDs = NULL, $limit = 0) { | |||
*/ | |||
public static function add($groupID) { | |||
// first delete the current cache | |||
self::remove($groupID); | |||
self::clearGroupContactCache($groupID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is causing the test failure, because $groupID is (incorrectly) an array here. remove()
said it accepted an int as the first argument, but could also accept an array. To fix this, I think you need to iterate through the array at line 208 so that add()
is only passed an int.
7b6638f
to
67bf42a
Compare
Almost all the code in the function was to support a path no longer used in core, or seamingly anywhere outside CiviHR.
67bf42a
to
6b05374
Compare
unrelated fail - @MegaphoneJon fixes are squashed into that last patch |
test this please |
@monishdeb @JoeMurray we should get this merged before you do any further work on groups! |
(I think it has been fairly thoroughly reviewed by @MegaphoneJon but it seems stalled - not quite sure why) |
My last issue was a "merge on pass" change, so it's fine with me if this gets merged. FWIW though - the rebase before approval makes it hard to review just the changes that were requested. I'm not sure if there's an optimal workflow here though. |
test this please |
test fail was unrelated |
@monishdeb @colemanw @totten @yashodha Jon has reviewed this & I altered the pr to incorporate Tim's suggestion - merger needed |
I did some thorough testing in my local and my feedback aligns with what others have commented above. Unit tests are covered, test build passes. Merging .. |
Thanks @monishdeb @MegaphoneJon - I think this is a big improvement in readability - turns out a lot of code wasn't doing much! |
Similarly |
loadAll is barely ever called ... and that is too often |
Hmm agree, called only on contact summary page >> on clicking smart group pane under 'Group' tab. This is a good improvement and hopefully will resolve the issues we faced during group-contact-cache-clear. Thanks @eileenmcnaughton for your work :) |
@monishdeb you can actually disable people's ability to expand that tab on sites with lots of smart groups - there is a setting - pretty sure @seamuslee001 uses it |
…p cache rebuild This patch is merged upstream & I am submitting it & civicrm#10943 to our codebase. Doing this a) syncs us with upstream in the area we are working on b) adds some code consolidation making it easier for us to tinker with the impact of cache tables. Note that this change switches from one sql call to many when clearing groups from the group_contact_cache table. I'm thinking this is probably not a problem for us as we have not that many groups (& hopefully we can further reduce them). Also the apparent COMMIT is a bit of a a myth - CRM_Core_Transaction does weird complex transaction nesting stuff... Change-Id: I3d6505b04af1134884f660d71316afb2490221d1
This is a combination of the commits in civicrm#10943 Bringing this in does not make any actual change for us. However, it makes it easier for us to play with the following options 1) setting cache mode to FALSE on a per process basis. Currently there are statics to try to ensure that caches are flushed only once per process, but without this the add to group & remove from group functions bypass them. From a script CRM_Core_Config::setPermitCacheFlushMode(FALSE); would prevent cache flushing in the session for group_contact_cache & acl caches until the script ends or it is retoggled. Also acl caches were not previously subject to the statics. 2)We would consider flushing caches by cron rather than during script runtime. There is a setting for that & we could extend to cover acl cache. Flushing by cron permits (optionally) the use of TRUNCATE - which appears to be faster for everyone in the world but us :-) Change-Id: Ib790939421d3b7ce5d2e258154a8015b8652234f
Overview
Add test, & consolidate code controlling cache clearance via the api. An alternate approach to #10905 by @MegaphoneJon. This also achieves Jon's original goal of less frequent cache clearing during cli imports
Before
CLI imports clear group cache once per line. No test
After
CLI imports clear group cache once. Test exists.
This also has the impact ok making cache clearing more consistent & overall slightly less as the
$config->doNotReset is respected by acl cache & group membership changes now too
Technical Details
CRM-21109 references a specific area of concern to reduce clearing of caches when running a CLI import & @MegaphoneJon has created #10905 to suggest that we set $config->doNotReset when looking into this.
My initial feeling was that we should allow 'cache instructions' to be passed in via api calls - ie. if you are doing a long running process then it should be possible to quash cache-clearing somewhat during this. As it turns out looking at the code it may also make sense to request more aggressive cache clearing in some (as yet undreamt) circumstances.
Comments
Looking at the code I have a number of notes - mostly due to refreshing my memory.
There are 3 caches that are potentially cleared when editing a contact:
The prevnext_cache never seems to be a performance concern
- The group contact cache goes through a number of checks before clearing
-5) after all this it will clear the cache, removing entries based on the timestamp of the group & the smartGroupCacheTimeout . There is an opportunity for optimisation in this query.
Based on the above I suspect it is more likely that if there is a WMF problem with the cache clearing it is the query running slow rather than it being too frequent
Opportunistic
To alter that setting & not clear the smart group in-process it is necessary to set the setting- potentially in civicrm.settings.php with the following:
It is also necessary to set up a cron to clear the caches outside of processes (calling api job.group_cache_flush). If the smart group cache time out is set to 0 TRUNCATE will be used in the cron (rather then DELETE FROM). This is faster for many sites, but not WMF
optimisation
Acl_contact_cache
This problem
Most of the above is by way of background. Specifically to this PR is the question
I realise there was some push back on cache management via the api in discussions about v4 api - but my sense is that was more about whether the api should do caching, rather than whether it should be able to tune-down cache clearing.
@totten @colemanw @xurizaemon @MegaphoneJon @ejegg ping