dev-core-166/lessen memory impact

[JO0st]

Overview

Making shure the job to update membership statuses can cope with large numbers of memberships

Before

The updateallmembershipstatus runs out of memory when processing large numbers of memberships.

After

updateallmemberships uses less memory

Technical Details

Instead of getting all memberships in memory and filtering them there to process only the relevant ones the filtering is done by altering the query before getting the memberships from the database.

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Thanks @JO0st.
I see that there are a couple of unit tests for this function in api_v3_JobTest - testProcessMembership and testProcessMembershipNoDeceasedStatus but we could definitely use more coverage of this function. Are you able to include a test or two to check the functionality you changed?

[JO0st]

I would love to add some tests, but have two problems to do so:

• I can't get the tests to run on my local machine, so making sure the tests are correct is going to be difficult. Unfortunatly I don't have the time to investigate why the tests aren't running.
• The only functionality I changed is making the capability to cope with large numbers of memberships. To test this functionality I need a test database with these numbers of memberships. I can only say that the function works on our database with about 1.3 million memberships.

[colemanw]

@JO0st I'm not requesting a performance test, just a test of the function to ensure that the same result set is returned before and after.

[davejenx]

@JO0st @colemanw @eileenmcnaughton I have added more tests for the process_membership job in #12918 & #12927 and tested with & without this PR. All tests passed before & after. Looking at the code changes, I wondered if the behaviour could differ for memberships inherited between individuals (e.g. parent-child) when one party dies, however I have not found any differences in behaviour in manual testing of this. The unit tests include checking inherited memberships, though not the inherited/deceased case.*

As an aside, I wasn't sure that Civi's current behaviour when a primary member dies is (morally) right: the inherited memberships are marked as deceased. Discussed at #12918.

• In the course of testing this PR, I killed Kenny (Kenny Jameson-González (deceased)).

[eileenmcnaughton]

@davejenx is this 'ready to merge' in your opinion?

[eileenmcnaughton]

I marked 'has-test' as tests were merged separately

[davejenx]

@eileenmcnaughton Yes, I think so.

[eileenmcnaughton]

woot!

[bjendres]

Are you aware that this change has an implied change in logic? The old code checked for

!$dao->is_override

while the new code moved that into SQL as

civicrm_membership.is_override IS NULL

and

civicrm_membership.is_override IS NOT NULL

But, at least in our setup, we have three values in this column: 0, NULL, and 1. If that is standard, it should then be:

(civicrm_membership.is_override IS NULL OR civicrm_membership.is_override = 0)

and

civicrm_membership.is_override = 1 

respectively.

Could anyone confirm this?

[davejenx]

@bjendres Yes, I think there was a subsequent PR addressing this, will try to find a reference...

Here are the later commits:

• 7fe259c
• 0060bed

Issue & PR:

• https://lab.civicrm.org/dev/core/issues/881
• change is_override logic #14059

[eileenmcnaughton]

We possibly should have targetted the rc for 881 - but it will be in the June release. @seamuslee001 @totten should we drop a 5.13 for 881?

[bjendres]

Thanks @davejenx, I hadn't seen that.

Wrt the release of 881: I'll start patching my clients' systems right away, but for other people it might be good to get this out asap, since it fails without error message - it simply doesn't change the status of some memberships any more....

[eileenmcnaughton]

Yep - we are looking at putting out a patch release with this & another change - product-mtce chat channel is the place to discuss

[bjendres]

Thanks, @eileenmcnaughton