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

Allow calls to opcache_reset to be disabled in config #19948

Merged
merged 8 commits into from Nov 15, 2022

Conversation

samjf
Copy link
Contributor

@samjf samjf commented Nov 4, 2022

Description:

ref: CLOUD-798

From time to time, we experience corrupt opcaches that cause outages which can cause various random errors to occur. After investigating we found that certain processes we use call Matomo console commands which result in the reset of the opcache. We believe it is likely that multiple concurrent opcache resets might result in the corrupt opcache.

This PR simply provides a configuration switch which can be used to disable opcache resets. This is off by default. When enabled this configuration will allow better control over the opcache and will likely allow us to avoid opcache corruption.

This configuration change was added in the cache section of the configuration as such:
Cache['disable_opcache_reset']

Review

@samjf samjf added the Needs Review For pull requests that need a code review. label Nov 4, 2022
@samjf samjf requested a review from tsteur November 4, 2022 02:22
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @samjf - just a couple of points but otherwise this looks pretty straightforward

  1. Normally we go for enable_ rather than disable_ for new configs, see https://developer.matomo.org/guides/piwiks-ini-configuration#naming
  2. This might be good to create a brief new FAQ for and/or at least mention in the developer changelog

@samjf
Copy link
Contributor Author

samjf commented Nov 7, 2022

Thanks @justinvelluppillai I'll make a fix for those and I'll keep those on file when making PRs in future.

@tsteur
Copy link
Member

tsteur commented Nov 8, 2022

@samjf samjf removed the Needs Review For pull requests that need a code review. label Nov 8, 2022
@samjf samjf added the Needs Review For pull requests that need a code review. label Nov 9, 2022
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add the 4.12.4 section to the changelog and move your change there we can include this in the release today 👍🏽

Other changes look good

CHANGELOG.md Outdated
@@ -27,6 +27,9 @@ The Product Changelog at **[matomo.org/changelog](https://matomo.org/changelog)*

* The methods `setExcludedReferrers` and `getExcludedReferrers` have been added to the JavaScript tracker. They allow setting and receiving the referrers the JavaScript tracker should ignore. If a referrer matches an entry on that list, it will not be passed with the tracking requests and the attribution cookie will stay unchanged. This can for example be used if you need to forward your users to an external service like SSO or payment and don't want any visits or conversions being attributed to those services.

### New config.ini.php settings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer in 4.12.0 but 4.12.4 (if we merge it now).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realise it was by patch version, but it makes sense hehe. I'll change this now.

@justinvelluppillai justinvelluppillai added this to the 4.12.4 milestone Nov 14, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one I've removed "general" from the changelog as it's not a GeneralConfig so that might be confusing, otherwise looks good to merge.

@justinvelluppillai justinvelluppillai merged commit 151ebfd into 4.x-dev Nov 15, 2022
@justinvelluppillai justinvelluppillai deleted the C798_addConfigToDisableOpcacheReset branch November 15, 2022 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review For pull requests that need a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants