Skip to content
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

fixes core#2687: regression on group rebuild scheduled job #20835

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

MegaphoneJon
Copy link
Contributor

Overview

Cron fails if you have "Rebuild Smart Group Cache" enabled in the default configuration. Detailed replication steps on the ticket: https://lab.civicrm.org/dev/core/-/issues/2687

Before

Cron fails (specifically, job.rebuild_group fails).

After

Cron succeeds.

Comments

This is a side effect of a refactoring in #20373.

@civibot
Copy link

civibot bot commented Jul 12, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@MegaphoneJon can you put this against the rc ?

@MegaphoneJon MegaphoneJon changed the base branch from master to 5.39 July 12, 2021 20:13
@civibot civibot bot added 5.39 and removed master labels Jul 12, 2021
@MegaphoneJon MegaphoneJon changed the base branch from 5.39 to 5.40 July 12, 2021 20:17
@civibot civibot bot added 5.40 and removed 5.39 labels Jul 12, 2021
@MegaphoneJon
Copy link
Contributor Author

done...once I remembered the rc is now 5.40

@MegaphoneJon
Copy link
Contributor Author

It's also reaching my attention that this configuration may have just been silently failing in the past. I'm a bit worried about what's going to happen when all of a sudden various sites with inefficient smart group queries start running them...

@eileenmcnaughton
Copy link
Contributor

@MegaphoneJon hmm - that's a good point - this job is one that should only be enabled with care on sites for which it is suitable. I recently realised people thought it was recommended to use this job - but I think the docs are clearer on that now.

How sure are you that it was silently failing?

@MegaphoneJon
Copy link
Contributor Author

Not 100% sure - but after deploying this fix to production on two sites, their cron jobs started failing. When I ran my smart group profiling script, I found smart groups that couldn't possibly have finished in a reasonable timeframe.

@eileenmcnaughton
Copy link
Contributor

@andrew-cormick-dockery that script ^^ would have been helpful for you in the past

@seamuslee001
Copy link
Contributor

@eileenmcnaughton @MegaphoneJon so where are we at with this then?, it makes sense to me but just wondering about the concern eileen raised

@eileenmcnaughton
Copy link
Contributor

We can put this in the dev digest & get more people to check

@kcristiano
Copy link
Member

This PR also makes sense to me. I took a look at a number of sites and those that are running this job, but did not change the limit are silently failing. Based on the docs https://docs.civicrm.org/user/en/latest/initial-set-up/scheduled-jobs/#job_group_cache_flush I've started to switch sites over to disabling this job and adding group_cache_flush

As we already have documentation, I think we could merge this and add the details in the release notes. I am not sure if we need an upgrade notice as well. I could go either way on that.

@eileenmcnaughton eileenmcnaughton merged commit 906d150 into civicrm:5.40 Jul 22, 2021
pcurrier added a commit to Nubay-Services/civicrm-core that referenced this pull request Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants