{"action":"created","issue":{"url":"https://api.github.com/repos/piwik/piwik/issues/7377","labels_url":"https://api.github.com/repos/piwik/piwik/issues/7377/labels{/name}","comments_url":"https://api.github.com/repos/piwik/piwik/issues/7377/comments","events_url":"https://api.github.com/repos/piwik/piwik/issues/7377/events","html_url":"https://github.com/piwik/piwik/pull/7377","id":60074655,"number":7377,"title":"refactor archive purging for clarity and resilience.","user":{"login":"diosmosis","id":125140,"avatar_url":"https://avatars.githubusercontent.com/u/125140?v=3","gravatar_id":"","url":"https://api.github.com/users/diosmosis","html_url":"https://github.com/diosmosis","followers_url":"https://api.github.com/users/diosmosis/followers","following_url":"https://api.github.com/users/diosmosis/following{/other_user}","gists_url":"https://api.github.com/users/diosmosis/gists{/gist_id}","starred_url":"https://api.github.com/users/diosmosis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/diosmosis/subscriptions","organizations_url":"https://api.github.com/users/diosmosis/orgs","repos_url":"https://api.github.com/users/diosmosis/repos","events_url":"https://api.github.com/users/diosmosis/events{/privacy}","received_events_url":"https://api.github.com/users/diosmosis/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/piwik/piwik/labels/Task","name":"Task","color":"bfe5bf"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/piwik/piwik/milestones/73","html_url":"https://github.com/piwik/piwik/milestones/Piwik%202.12.0","labels_url":"https://api.github.com/repos/piwik/piwik/milestones/73/labels","id":852680,"number":73,"title":"Piwik 2.12.0","description":"This release ideally should not include DB schema upgrades.\r\n","creator":{"login":"mattab","id":466765,"avatar_url":"https://avatars.githubusercontent.com/u/466765?v=3","gravatar_id":"","url":"https://api.github.com/users/mattab","html_url":"https://github.com/mattab","followers_url":"https://api.github.com/users/mattab/followers","following_url":"https://api.github.com/users/mattab/following{/other_user}","gists_url":"https://api.github.com/users/mattab/gists{/gist_id}","starred_url":"https://api.github.com/users/mattab/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mattab/subscriptions","organizations_url":"https://api.github.com/users/mattab/orgs","repos_url":"https://api.github.com/users/mattab/repos","events_url":"https://api.github.com/users/mattab/events{/privacy}","received_events_url":"https://api.github.com/users/mattab/received_events","type":"User","site_admin":false},"open_issues":45,"closed_issues":17,"state":"open","created_at":"2014-11-04T11:19:45Z","updated_at":"2015-03-11T00:06:21Z","due_on":"2015-03-18T11:00:00Z","closed_at":null},"comments":9,"created_at":"2015-03-06T08:47:17Z","updated_at":"2015-03-11T00:10:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/piwik/piwik/pulls/7377","html_url":"https://github.com/piwik/piwik/pull/7377","diff_url":"https://github.com/piwik/piwik/pull/7377.diff","patch_url":"https://github.com/piwik/piwik/pull/7377.patch"},"body":"Refs #7181 \r\n\r\n**Purpose**\r\n\r\nThere are two purposes behind the refactoring in this pull request:\r\n\r\n* **clarity**: to increase the ability for developers to understand the purging system, both in terms of how it is used and how it works. This involves better scoped classes, better documentation/comments and better class names.\r\n\r\n* **resilience**: to increase resilience of the system, by decoupling intra-system dependencies and providing coping methods for eventual failures.\r\n\r\n**Description of Purging System**\r\n\r\nHere is a description of the refactored purging system:\r\n\r\n```\r\n   Users               +---------------------+             +---------------------------------+          +-------------+\r\n+-------------+        |                     |    adds to  |                                 | consumes |             |\r\n| API         |  calls | Archive\\Invalidator +------+------> SitesToReprocessDistributedList <----------+ CronArchive |\r\n|  +          +-------->                     |      |      |     (holds site IDs)            |          |             |\r\n|  |          |        +---------------------+      |      +----------------+----------------+          +-------------+\r\n|  +          |                   v -updates        |                       v                                          \r\n| Archive.php |                archive              |                    option                                        \r\n|  +          |                  table              |                      table                                       \r\n|  |          |                   ^ -deletes        |                       ^                                          \r\n|  +          |  calls +---------------------+      |      +----------------+----------------+          +------------------------+\r\n| Commands    +-------->                     |      +------>                                 | consumes |                        |\r\n+-------------+        | Archive\\Purger      |             | ArchivesToPurgeDistributedList  <----------+ Purging Scheduled Task |\r\n                       |                     +             |     (holds year months)         |          |                        |\r\n                       +----------^----------+             +---------------------------------+          +--------+---------------+\r\n                                  |                                     uses                                     |\r\n                                  +-------------------------------------------------------------------------------\r\n```\r\n\r\nUsers interact primarily w/ Archive\\Invalidator & Archive\\Purger services.\r\n\r\n* **Archive\\Invalidator**:\r\n  * invalidate: when archives are invalidated, the invalidator changes the archive done flags to `ArchiveWriter::DONE_INVALIDATED`. Then it adds the archive's site to the SitesToReprocess list and the archive's table-month to the ArchivesToPurge list.\r\n  * invalidate later: adds information identifying an archive to a distributed list (not pictured above) which is later read and its elements sent to the `invalidate` operation.\r\n    _Note: I did not modify this part of the code._\r\n\r\n* **Archive\\Purger**:\r\n  * purge invalidated: when purging invalidated archives is requested, the purger will look through the requested table (an archive table must be supplied) for invalidated archives that have newer, valid archives, and deletes them. Before doing this (which depends on joining the table w/ itself), a query is issued to find all site IDs in the table for invalidated archives. This is done only for performance, so the INNER JOIN query does not look through every row in the table.\r\n    valid archives are created after a site is reprocessed by CronArchive.\r\n  * purge others: other archive purging (temporary + custom ranges) do not depend on distributed lists. they just issue queries and delete archives.\r\n\r\n* **CronArchive**:\r\n  * when cron archiving is run, the SitesToReprocessDistributedList is read. Every site referenced by the list is reprocessed, which should create new valid archives, thus allowing the purger to purge the invalidated archives.\r\n\r\n** **Purging Scheduled Task**\r\n  * the daily scheduled task that purges invalidated archives will read from the ArchivesToPurgeDistributedList, and purge invalidated archives from the tables indicated. Since there can be many archive tables, and most of them won't have invalidated archives, we depend on the distributed list for efficiency. That is the sole reason for the list's existence and use.\r\n\r\n**List of changes**\r\n\r\n1. Made ArchivePurger + ArchiveInvalidator services and moved them out of core\\DataAccess. Since they access & manipulate different kinds of data in different contexts, they are not simple DAOs.\r\n2. Moved purge skipping logic to scheduled task for purging, since it is only required there. This allows other code to force a purge if necessary.\r\n3. Added command core:purge-old-archive-data so users can force archive purging.\r\n4. Decouple invalidated archive purging from site entity. Originally, site IDs of invalidated archives were stored in the ArchivesToPurge list, now, instead of depending on values in the option table, we do a select on the archive table to find the sites first. Then we do the expensive query using the site IDs.\r\n5. Decouple ArchivePurger from the ArchivesToPurgeDistributedList, so the class is not itself aware of the distributed list. Nor is it required for the list to be used when purging archive data.\r\n6. Split InvalidatedReports class into three classes (one is a common base type). The InvalidatedReports class was used to mange two distributed lists stored in the option table. The refactor splits them up into two classes for each list (ArchivesToPurgeDistributedList & SitesToReprocessDistributedList) + a common base type for a simple distributed list (core\\Concurrency\\DistributedList.php).\r\n  * List classes were moved to the systems that were their primary consumers. SitesToReprocessDistributedList was moved to core\\CronArchive & ArchivesToPurgeDistributedList to CoreAdminHome plugin for the scheduled task.\r\n  * The base type DistributedList does exactly what InvalidatedReports did before, nothing more or less. It does not introduce more logic (save one method, removeByIndex).\r\n7. Remove custom range archive purging from outdated archive purging, since they are not the same thing.\r\n8. Added lots of tests.\r\n\r\nAlso includes this change (for testing the command by injecting a mock command into core\\Console.php): a73225f\r\n\r\n**Resilience**\r\n\r\nResilience of the system is described in three ways, how many ways each part of the system can fail, how many ways for the users to cope if a part of the system fails, and how easy it is to debug potential bugs.\r\n\r\n* **Archive\\Invalidator**\r\n  * **failures**: can fail if hit by race condition when adding to distributed lists.\r\n\r\n    On failure to add to SitesToReprocess, user can run core:archive for the site, HOWEVER, there may be no easy way for the user to find which sites require this. A query on tables for invalidated rows w/o new data & w/o site entries in distributed list is required.\r\n\r\n    On failure to add to ArchivesToPurge, user can purge manually using new command, HOWEVER, there may be no easy way for the user to find which archive tables have invalided archives. Purging however is not as big an issue, since the user will notice table size problems, and use the command to deal w/ them.\r\n\r\n  * **to debug**: there is no way to debug the code easily at this point. Logging can be added, since invalidation does quite a lot of different stuff, but figuring out the specific type of logging is non-trivial. How can we find out what went wrong, BEFORE we tell a user to enable debug logging?\r\n\r\n* **Archive\\Purger**\r\n  * **failures**: can fail if it deletes more than it should.\r\n\r\n    On failure to delete correct archives, user can run core:archive, if the log data remains. If log data does not remain, there is no way for the system to cope.\r\n\r\n  * **to debug**: also no way to debug the code easily. Need to add logging.\r\n\r\n* **Purging Scheduled Task**\r\n  * **failures**: can fail if it deletes more than it should and UI shows 'no data' or when, and can fail if scheduled tasks are not run.\r\n\r\n    On failure to delete correct archives, user can run core:archive. No immediate action in the UI, though.\r\n\r\n    On failure of task scheduler, user can manually purge to deal w/ table sizes.\r\n\r\n  * **to debug**: First failure can be debugged in same way as Archive\\Purger failure. Second failure cannot be debugged due to current task scheduler system.\r\n\r\nPossible future TODO:\r\n\r\n* We could add a diagnostic command to look for invalidated archives that do not have new data.\r\n* We might want to make it easy to backup archive tables, since they contain the useful report data. Perhaps this could augment the log purging PrivacyManager feature.\r\n* The ability to re-process an individual report through the UI that forces archiving for a SINGLE report (regardless of whether browser archiving is enabled), can help w/ any 'no data for this report' inconsistencies.\r\n\r\n@piwik/core-team Ready for review."},"comment":{"url":"https://api.github.com/repos/piwik/piwik/issues/comments/78176192","html_url":"https://github.com/piwik/piwik/pull/7377#issuecomment-78176192","issue_url":"https://api.github.com/repos/piwik/piwik/issues/7377","id":78176192,"user":{"login":"mattab","id":466765,"avatar_url":"https://avatars.githubusercontent.com/u/466765?v=3","gravatar_id":"","url":"https://api.github.com/users/mattab","html_url":"https://github.com/mattab","followers_url":"https://api.github.com/users/mattab/followers","following_url":"https://api.github.com/users/mattab/following{/other_user}","gists_url":"https://api.github.com/users/mattab/gists{/gist_id}","starred_url":"https://api.github.com/users/mattab/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mattab/subscriptions","organizations_url":"https://api.github.com/users/mattab/orgs","repos_url":"https://api.github.com/users/mattab/repos","events_url":"https://api.github.com/users/mattab/events{/privacy}","received_events_url":"https://api.github.com/users/mattab/received_events","type":"User","site_admin":false},"created_at":"2015-03-11T00:10:57Z","updated_at":"2015-03-11T00:10:57Z","body":"Ok, I'm going to review it shortly"}}