WordPress.org

GlotPress

Opened 3 years ago

Closed 7 months ago

Last modified 7 months ago

#208 closed defect (fixed)

String counts caching issues

Reported by: dimadin Owned by: markoheijnen
Milestone: 1.0 Priority: normal
Version: Component: Cache
Keywords: has-patch Cc: juanramon.diaz@…, daisuke@…

Description

After numerous reports on WP Polyglots, I've looked into the GlotPress code to see what and where we're caching and when that cache is cleared.

There are two caches that we have and that affect us: active_originals_count_by_project_id which is a number of active strings in a project, and translation_set_status_breakdown, which is per translation set cache that contains counts of each status of string except of untranslated strings.

First cache is cleared when originals are imported into the project and that is fine. Second cache is cleared when new translation is added but there are actions that don't clear cache and I believe this is what is causing problems.

Translation set's cache isn't cleared when single string translation status is changed, when originals are imported, and when translation set is copied from another. Second case is responsible for weird numbers like negative number of untranslated strings. (this probably happens when approved strings become obsolete during import of originals: in that process count of total strings is refreshed while count of untranslated strings is dynamically generated as difference between total strings (from clean cache) and count of translated strings (from old cache), and if there is larger number of translated strings in old cache than new number of total strings, we get negative number)

So I suggest that we clear translation set's cache in functions I linked above.

Thoughts?

Attachments (2)

gp_cache_fix.patch (6.6 KB) - added by dimadin 3 years ago.
fix-invalidate-count.diff (2.6 KB) - added by markoheijnen 7 months ago.
Move invalidation to GP_Translation

Download all attachments as: .zip

Change History (24)

comment:1 @dimadin3 years ago

  • Keywords has-patch added

I created a patch. It doesn't just include cache cleaning on missing places, but it also moves all cleaning to hooks. Why? Because that way other tools can benefit too. For example, on wp.org we can have a plugin that will clean language packages when changes are made. Or there can be notification plugin when actions are made etc.

Not tested with object cache enabled.

@dimadin3 years ago

comment:2 @defries3 years ago

  • Milestone set to 1.0

comment:3 @markoheijnen2 years ago

  • Component changed from performance to Cache

Will be looking into this after WordSesh.

comment:4 @markoheijnen2 years ago

  • Owner set to markoheijnen
  • Status changed from new to accepted

comment:5 @markoheijnen2 years ago

In 777:

First pass on fixing the translations set count cache. Also fixes setting strings obsolete when they already are obsolete

props dimadin. See #208

comment:6 @markoheijnen2 years ago

Just did a first pass on fixing this. Didn't used the hooks for clearing the cache since you shouldn't be able to unhook them. So this commit mainly fix clearing the cache when new strings are imported.

Fix minor issue with strings getting set as obsolete when they already are obsolete. Needed to fix that for the counter otherwise the cache would always be cleared when there is already one obsolete string.

Delete cache inside copy_translations_from() shouldn't be needed so didn't add that one. Maybe we should request non cache data there. Not sure yet about the one inside edit_single_translation().

comment:7 @markoheijnen2 years ago

In 785:

Renamed cache functions names to be more like WordPress naming conventions. See #208

comment:8 @juanramondiaz2 years ago

  • Cc juanramon.diaz@… added

comment:9 @markoheijnen2 years ago

In 788:

Added two actions: originals_imported and translations_imported. See #208

comment:10 @markoheijnen2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Closing the ticket as fixed. If something is missing or wrong please reopen it.

comment:11 @petya8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

BG_bg falsely shows 2 waiting strings and is at 99% here, but there are actually no waiting strings here.

This is the previous issue we had with it two weeks ago: https://make.wordpress.org/polyglots/2014/10/12/bg-bg-is-not-available-in-the-installation/#comment-259397

comment:12 @slackbot8 months ago

This ticket was mentioned in Slack in #polyglots by extendwings. View the logs.

comment:13 @andg8 months ago

Kind of the same goes for it_IT as well, showing even negative numbers. Might be related to the fact that one of the packs wasn't correctly built, apparently: https://make.wordpress.org/polyglots/2014/10/13/on-our-support-forum-weve-been-informed-that/.

comment:14 @extendwings8 months ago

  • Cc daisuke@… added

@markoheijnen7 months ago

Move invalidation to GP_Translation

comment:15 @markoheijnen7 months ago

Currently we don't invalidate the cache when we change the status through an AJAX call. When looking at the code, it didn't make sense that the router was invalidating the cache. I moved the functionality to GP_Translation what makes more sense to me.

This should fix the issue that the counts don't get cleared after someone approves a string.

comment:16 @markoheijnen7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 993:

Invalidate the translation set cache when approving/rejecting a string.

This includes moving the invalidation code from the router to GP_Translation. This is the lower level API where the action is taken place.

Fixes #208

comment:17 @nbachiyski7 months ago

Another cache value of interest might be active_originals_count_by_project_id, which, as the name suggest is by project id.

The other day I was successfully making the proper counts to appear, using this script:

        foreach( $projects as $project ) {
            wp_cache_delete( $project->id, 'active_originals_count_by_project_id' );
        }

        foreach( $sets as $set ) {
            wp_cache_delete( $set->id, 'translation_set_status_breakdown' );
        }

comment:18 @markoheijnen7 months ago

active_originals_count_by_project_id looks fine to me since it's the value of total amount of strings in a project which get's invalidated on import. The problem was that the translation_set_status_breakdown cache value wasn't deleted. This happens in gp_clean_translation_sets_cache().

What does wonder me if we should rename gp_clean_translation_sets_cache to gp_clean_project_cache and then include cleaning active_originals_count_by_project_id.

Also we can delete the gp_clean_translation_set_cache() call in GP_Translation_Set since it already happens in GP_translation. That said, it's one call more to the many calls GP_Translation will make.

comment:19 @markoheijnen7 months ago

In 994:

Don't flush the translation cache when copying a translation set. See #208

comment:20 @markoheijnen7 months ago

In 995:

Add basic unit tests to check if the cache is being invalidated when a translation is approved.

See #208

comment:21 @slackbot7 months ago

This ticket was mentioned in Slack in #glotpress by markoheijnen. View the logs.

comment:22 @markoheijnen7 months ago

In 998:

Check if translation_set_id has been passed when creating a translation. See #208

Note: See TracTickets for help on using tickets.