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

Move Archive.php archive invalidation to Loader… #15616

Merged
merged 19 commits into from
Mar 10, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 4 additions & 72 deletions core/Archive.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
use Piwik\Archive\ArchiveQueryFactory;
use Piwik\Archive\Parameters;
use Piwik\ArchiveProcessor\Rules;
use Piwik\Archive\ArchiveInvalidator;
use Piwik\Container\StaticContainer;
use Piwik\DataAccess\ArchiveSelector;

Expand Down Expand Up @@ -167,11 +166,6 @@ class Archive implements ArchiveQuery
*/
private static $cache;

/**
* @var ArchiveInvalidator
*/
private $invalidator;

/**
* @param Parameters $params
* @param bool $forceIndexedBySite Whether to force index the result of a query by site ID.
Expand All @@ -183,8 +177,6 @@ public function __construct(Parameters $params, $forceIndexedBySite = false,
$this->params = $params;
$this->forceIndexedBySite = $forceIndexedBySite;
$this->forceIndexedByDate = $forceIndexedByDate;

$this->invalidator = StaticContainer::get('Piwik\Archive\ArchiveInvalidator');
}

/**
Expand Down Expand Up @@ -453,67 +445,6 @@ public static function createDataTableFromArchive($recordName, $idSite, $period,
return $dataTable;
}

private function getSiteIdsThatAreRequestedInThisArchiveButWereNotInvalidatedYet()
{
if (is_null(self::$cache)) {
self::$cache = Cache::getTransientCache();
}

$id = 'Archive.SiteIdsOfRememberedReportsInvalidated';

if (!self::$cache->contains($id)) {
self::$cache->save($id, array());
}

$siteIdsAlreadyHandled = self::$cache->fetch($id);
$siteIdsRequested = $this->params->getIdSites();

foreach ($siteIdsRequested as $index => $siteIdRequested) {
$siteIdRequested = (int) $siteIdRequested;

if (in_array($siteIdRequested, $siteIdsAlreadyHandled)) {
unset($siteIdsRequested[$index]); // was already handled previously, do not do it again
} else {
$siteIdsAlreadyHandled[] = $siteIdRequested; // we will handle this id this time
}
}

self::$cache->save($id, $siteIdsAlreadyHandled);

return $siteIdsRequested;
}

private function invalidatedReportsIfNeeded()
{
$siteIdsRequested = $this->getSiteIdsThatAreRequestedInThisArchiveButWereNotInvalidatedYet();

if (empty($siteIdsRequested)) {
return; // all requested site ids were already handled
}

$sitesPerDays = $this->invalidator->getRememberedArchivedReportsThatShouldBeInvalidated();

foreach ($sitesPerDays as $date => $siteIds) {
if (empty($siteIds)) {
continue;
}

$siteIdsToActuallyInvalidate = array_intersect($siteIds, $siteIdsRequested);

if (empty($siteIdsToActuallyInvalidate)) {
continue; // all site ids that should be handled are already handled
}

try {
$this->invalidator->markArchivesAsInvalidated($siteIdsToActuallyInvalidate, array(Date::factory($date)), false);
} catch (\Exception $e) {
Site::clearCache();
throw $e;
}
}

Site::clearCache();
}

/**
* Queries archive tables for data and returns the result.
Expand Down Expand Up @@ -638,8 +569,6 @@ private function getArchiveIds($archiveNames)
*/
private function cacheArchiveIdsAfterLaunching($archiveGroups, $plugins)
{
$this->invalidatedReportsIfNeeded();

$today = Date::today();

foreach ($this->params->getPeriods() as $period) {
Expand Down Expand Up @@ -856,8 +785,11 @@ public static function getPluginForReport($report)
*/
private function prepareArchive(array $archiveGroups, Site $site, Period $period)
{
// if cron archiving is running, we will invalidate in CronArchive, not here
$invalidateBeforeArchiving = !SettingsServer::isArchivePhpTriggered();

$parameters = new ArchiveProcessor\Parameters($site, $period, $this->params->getSegment());
$archiveLoader = new ArchiveProcessor\Loader($parameters);
$archiveLoader = new ArchiveProcessor\Loader($parameters, $invalidateBeforeArchiving);

$periodString = $period->getRangeString();

Expand Down
84 changes: 71 additions & 13 deletions core/ArchiveProcessor/Loader.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,18 @@
*/
namespace Piwik\ArchiveProcessor;

use Piwik\Archive\ArchiveInvalidator;
use Piwik\Cache;
use Piwik\Config;
use Piwik\Container\StaticContainer;
use Piwik\Context;
use Piwik\DataAccess\ArchiveSelector;
use Piwik\DataAccess\ArchiveTableCreator;
use Piwik\Date;
use Piwik\Db;
use Piwik\Piwik;
use Piwik\Site;
use Psr\Log\LoggerInterface;

/**
* This class uses PluginsArchiver class to trigger data aggregation and create archives.
Expand All @@ -33,9 +38,28 @@ class Loader
*/
protected $params;

public function __construct(Parameters $params)
/**
* @var ArchiveInvalidator
*/
private $invalidator;

/**
* @var \Piwik\Cache\Cache
*/
private $cache;

/**
* @var LoggerInterface
*/
private $logger;

public function __construct(Parameters $params, $invalidateBeforeArchiving = false)
{
$this->params = $params;
$this->invalidateBeforeArchiving = $invalidateBeforeArchiving;
$this->invalidator = StaticContainer::get(ArchiveInvalidator::class);
$this->cache = Cache::getTransientCache();
$this->logger = StaticContainer::get(LoggerInterface::class);
}

/**
Expand Down Expand Up @@ -65,11 +89,19 @@ private function prepareArchiveImpl($pluginName)
{
$this->params->setRequestedPlugin($pluginName);

list($idArchive, $visits, $visitsConverted) = $this->loadExistingArchiveIdFromDb();
if (!empty($idArchive)) {
list($idArchive, $visits, $visitsConverted, $isAnyArchiveExists) = $this->loadExistingArchiveIdFromDb();
if (!empty($idArchive)) { // we have a usable idarchive (it's not invalidated and it's new enough)
return $idArchive;
}

// if there is an archive, but we can't use it for some reason, invalidate existing archives before
// we start archiving. if the archive is made invalid, we will correctly re-archive below.
if ($this->invalidateBeforeArchiving
&& $isAnyArchiveExists
) {
$this->invalidatedReportsIfNeeded();
}

/** @var ArchivingStatus $archivingStatus */
$archivingStatus = StaticContainer::get(ArchivingStatus::class);
$archivingStatus->archiveStarted($this->params);
Expand Down Expand Up @@ -170,20 +202,17 @@ protected function isArchivingForcedToTrigger()
*/
public function loadExistingArchiveIdFromDb()
{
$noArchiveFound = array(false, false, false);

if ($this->isArchivingForcedToTrigger()) {
return $noArchiveFound;
}
$this->logger->debug("Archiving forced to trigger for {$this->params}.");

$minDatetimeArchiveProcessedUTC = $this->getMinTimeArchiveProcessed();
$idAndVisits = ArchiveSelector::getArchiveIdAndVisits($this->params, $minDatetimeArchiveProcessedUTC);

if (!$idAndVisits) {
return $noArchiveFound;
// return no usable archive found, and no existing archive. this will skip invalidation, which should
// be fine since we just force archiving.
return [false, false, false, false];
}

return $idAndVisits;
$minDatetimeArchiveProcessedUTC = $this->getMinTimeArchiveProcessed();
$result = ArchiveSelector::getArchiveIdAndVisits($this->params, $minDatetimeArchiveProcessedUTC);
return $result;
}

/**
Expand Down Expand Up @@ -242,4 +271,33 @@ private function getIdSitesToArchiveWhenNoVisits()

return $cache->fetch($cacheKey);
}

private function invalidatedReportsIfNeeded()
Copy link
Member

Choose a reason for hiding this comment

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

@diosmosis might be good to add some tests for this maybe? Like it could return the invalidated date/siteIds which we then can test against?

Copy link
Member Author

Choose a reason for hiding this comment

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

added tests

{
$sitesPerDays = $this->invalidator->getRememberedArchivedReportsThatShouldBeInvalidated();

foreach ($sitesPerDays as $date => $siteIds) {
if (empty($siteIds)
|| !in_array($this->params->getSite()->getId(), $siteIds)
) {
continue;
}

$date = Date::factory($date);
if ($date->isEarlier($this->params->getPeriod()->getDateStart())
|| $date->isLater($this->params->getPeriod()->getDateEnd())
) {
continue; // date in list is not the current date, so ignore it
}

try {
$this->invalidator->markArchivesAsInvalidated([$this->params->getSite()->getId()], array($date), false, $this->params->getSegment());
} catch (\Exception $e) {
Site::clearCache();
throw $e;
}
}

Site::clearCache();
}
}
5 changes: 5 additions & 0 deletions core/ArchiveProcessor/Parameters.php
Original file line number Diff line number Diff line change
Expand Up @@ -258,4 +258,9 @@ public function setIsRootArchiveRequest($isRootArchiveRequest)
{
$this->isRootArchiveRequest = $isRootArchiveRequest;
}

public function __toString()
{
return "[idSite = {$this->getSite()->getId()}, period = {$this->getPeriod()->getLabel()} {$this->getPeriod()->getRangeString()}, segment = {$this->getSegment()->getString()}]";
}
}
34 changes: 32 additions & 2 deletions core/CronArchive.php
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,16 @@ private function isThereAValidArchiveForPeriod($idSite, $period, $date, $segment
$periodsToCheck = [Factory::build($period, $date, Site::getTimezoneFor($idSite))];
}

$isTodayIncluded = $this->isTodayIncludedInPeriod($idSite, $periodsToCheck);
$isLast = preg_match('/^last([0-9]+)/', $date, $matches);

// don't do this check for a single period that includes today
if ($isTodayIncluded
Copy link
Member

Choose a reason for hiding this comment

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

@diosmosis not 100% understanding this part. today would be mostly included right? I understand that we can assume basically there is no valid archive for today because we archive each time anyway. I'm just wondering if it would still need to process below code in case

  • Cron Archive starts
  • The default date is last2
  • We invalidate all reports
  • We are about to archive a site
  • This logic runs
  • Now if previously some reports from 10 days ago are invalidated, we would need to change it to last10 basically

Not sure it's clear what I mean? Or maybe this is not needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea here was if it was a single period like, period = week, day = today, then we don't check for an existing usable archive. If it is for a date in the past, then we check if there is a usable archive and if so we don't need to archive.

For lastN values, however, it is more complicated. Here, we want to check for every date except today. So we check for valid archives. If the date is last5 and for the 5th, 4th and 3rd day we have valid archives, then we change it to last2. The only difference is if we have valid archives for every day, we still do last1 (I think the min is last1 now but we can change it back to last2).

Maybe I can add some tests for this code too.

Copy link
Member Author

Choose a reason for hiding this comment

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

added tests, maybe you can better see what it's trying to do

Copy link
Member

Choose a reason for hiding this comment

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

fully getting it now 👍

&& !$isLast
) {
return [false, null];
}

$periodsToCheckRanges = array_map(function (Period $p) { return $p->getRangeString(); }, $periodsToCheck);

$this->invalidateArchivedReportsForSitesThatNeedToBeArchivedAgain();
Expand All @@ -998,13 +1008,13 @@ private function isThereAValidArchiveForPeriod($idSite, $period, $date, $segment
}

$diff = array_diff($periodsToCheckRanges, $foundArchivePeriods);
$isThereArchiveForAllPeriods = empty($diff);
$isThereArchiveForAllPeriods = empty($diff) && !$isTodayIncluded;

// if there is an invalidated archive within the range, find out the oldest one and how far it is from today,
// and change the lastN $date to be value so it is correctly re-processed.
$newDate = $date;
if (!$isThereArchiveForAllPeriods
&& preg_match('/^last([0-9]+)/', $date, $matches)
&& $isLast
) {
$lastNValue = (int) $matches[1];

Expand Down Expand Up @@ -1033,6 +1043,26 @@ private function isThereAValidArchiveForPeriod($idSite, $period, $date, $segment
return [$isThereArchiveForAllPeriods, $newDate];
}

/**
* @param int $idSite
* @param Period[] $periods
* @return bool
* @throws Exception
*/
private function isTodayIncludedInPeriod($idSite, $periods)
{
$timezone = Site::getTimezoneFor($idSite);
$today = Date::factoryInTimezone('today', $timezone);

foreach ($periods as $period) {
if ($period->isDateInPeriod($today)) {
return true;
}
}

return false;
}

/**
* @param $idSite
* @return array
Expand Down
Loading