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.

Sign up for GitHub

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

Jump to bottom

Use log tables to decide which tables can be purged #10304

Merged
merged 3 commits into from
Jul 17, 2016
Merged

Use log tables to decide which tables can be purged #10304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bea00fc
use log tables to decide which tables can be purged
tsteur Jul 13, 2016
daa423f
make sure to return the number of deleted visits
tsteur Jul 13, 2016
d30476c
fix tests
tsteur Jul 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 2 additions & 33 deletions core/DataAccess/RawLogDao.php
View file
Open in desktop
Expand Up @@ -113,47 +113,16 @@ public function forAllLogs($logTable, $fields, $conditions, $iterationStep, $cal
} while (count($rows) == $iterationStep);
}

/**
* Deletes visits with the supplied IDs from log_visit. This method does not cascade, so rows in other tables w/
* the same visit ID will still exist.
*
* @param int[] $idVisits
* @return int The number of deleted rows.
*/
public function deleteVisits($idVisits)
{
$sql = "DELETE FROM `" . Common::prefixTable('log_visit') . "` WHERE idvisit IN "
. $this->getInFieldExpressionWithInts($idVisits);

$statement = Db::query($sql);
return $statement->rowCount();
}

/**
* Deletes visit actions for the supplied visit IDs from log_link_visit_action.
*
* @param int[] $visitIds
* @return int The number of deleted rows.
*/
public function deleteVisitActionsForVisits($visitIds)
{
$sql = "DELETE FROM `" . Common::prefixTable('log_link_visit_action') . "` WHERE idvisit IN "
. $this->getInFieldExpressionWithInts($visitIds);

$statement = Db::query($sql);
return $statement->rowCount();
}

/**
* Deletes conversions for the supplied visit IDs from log_conversion. This method does not cascade, so
* conversion items will not be deleted.
*
* @param int[] $visitIds
* @return int The number of deleted rows.
*/
public function deleteConversions($visitIds)
public function deleteFromLogTable($tableName, $visitIds)
Copy link
Member Author

Choose a reason for hiding this comment

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

this was all duplicated code

{
$sql = "DELETE FROM `" . Common::prefixTable('log_conversion') . "` WHERE idvisit IN "
$sql = "DELETE FROM `" . Common::prefixTable($tableName) . "` WHERE idvisit IN "
. $this->getInFieldExpressionWithInts($visitIds);

$statement = Db::query($sql);
Expand Down
44 changes: 18 additions & 26 deletions core/LogDeleter.php
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@
namespace Piwik;

use Piwik\DataAccess\RawLogDao;
use Piwik\Plugin\LogTablesProvider;

/**
* Service that deletes log entries. Methods in this class cascade, so deleting visits will delete visit actions,
Expand All @@ -21,9 +22,15 @@ class LogDeleter
*/
private $rawLogDao;

public function __construct(RawLogDao $rawLogDao)
/**
* @var LogTablesProvider
*/
private $logTablesProvider;

public function __construct(RawLogDao $rawLogDao, LogTablesProvider $logTablesProvider)
{
$this->rawLogDao = $rawLogDao;
$this->logTablesProvider = $logTablesProvider;
}

/**
Expand All @@ -35,33 +42,18 @@ public function __construct(RawLogDao $rawLogDao)
*/
public function deleteVisits($visitIds)
{
$this->deleteConversions($visitIds);
$this->rawLogDao->deleteVisitActionsForVisits($visitIds);

return $this->rawLogDao->deleteVisits($visitIds);
}
$numDeletedVisits = 0;

/**
* Deletes conversions by visit ID. This method cascades, so conversion items are also deleted.
*
* @param int[] $visitIds The list of visits to delete conversions for.
* @return int The number rows deleted.
*/
public function deleteConversions($visitIds)
{
$this->deleteConversionItems($visitIds);
return $this->rawLogDao->deleteConversions($visitIds);
}
foreach ($this->logTablesProvider->getAllLogTables() as $logTable) {
if ($logTable->getColumnToJoinOnIdVisit()) {
$numVisits = $this->rawLogDao->deleteFromLogTable($logTable->getName(), $visitIds);
if ($logTable->getName() === 'log_visit') {
$numDeletedVisits = $numVisits;
}
}
}

/**
* Deletes conversion items by visit ID.
*
* @param int[] $visitIds The list of visits to delete conversions for.
* @return int The number rows deleted.
*/
public function deleteConversionItems($visitIds)
{
return $this->rawLogDao->deleteConversionItems($visitIds);
return $numDeletedVisits;
Copy link
Member Author

@tsteur tsteur Jul 15, 2016 •
edited

Choose a reason for hiding this comment

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

before we returned only the number of visits deleted in log_visit but now we return the sum of deleted visits. I had a look and the value returned here seems to be actually not used (not even in the output of the console command anymore) so it shouldn't change anything but only "be more accurate" to return the sum of deleted entries

}

/**
Expand Down
18 changes: 13 additions & 5 deletions plugins/PrivacyManager/LogDataPurger.php
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@
namespace Piwik\Plugins\PrivacyManager;

use Piwik\Common;
use Piwik\Container\StaticContainer;
use Piwik\DataAccess\RawLogDao;
use Piwik\Date;
use Piwik\Db;
Expand Down Expand Up @@ -100,7 +101,7 @@ public function getPurgeEstimate($deleteLogsOlderThan)
// deal w/ log tables that will be purged
$maxIdVisit = $this->getDeleteIdVisitOffset($deleteLogsOlderThan);
if (!empty($maxIdVisit)) {
foreach ($this->getDeleteTableLogTables() as $table) {
foreach (self::getDeleteTableLogTables() as $table) {
// getting an estimate for log_action is not supported since it can take too long
if ($table != Common::prefixTable('log_action')) {
$rowCount = $this->getLogTableDeleteCount($table, $maxIdVisit);
Expand Down Expand Up @@ -150,13 +151,20 @@ private function getLogTableDeleteCount($table, $maxIdVisit)
// let's hardcode, since these are not dynamically created tables
public static function getDeleteTableLogTables()
{
$result = Common::prefixTables('log_conversion',
'log_link_visit_action',
'log_visit',
'log_conversion_item');
$provider = StaticContainer::get('Piwik\Plugin\LogTablesProvider');

$result = array();
foreach ($provider->getAllLogTables() as $logTable) {

if ($logTable->getColumnToJoinOnIdVisit()) {
$result[] = Common::prefixTable($logTable->getName());
}
}

if (Db::isLockPrivilegeGranted()) {
$result[] = Common::prefixTable('log_action');
}

return $result;
}
}
3 changes: 2 additions & 1 deletion plugins/PrivacyManager/tests/Integration/DataPurgingTest.php
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
use Piwik\Plugins\PrivacyManager\PrivacyManager;
use Piwik\Plugins\PrivacyManager\ReportsPurger;
use Piwik\Plugins\VisitorInterest\API as APIVisitorInterest;
use Piwik\Tests\Framework\Mock\Plugin\LogTablesProvider;
use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
use Piwik\Tracker\GoalManager;
use Piwik\Tests\Framework\Fixture;
Expand Down Expand Up @@ -517,7 +518,7 @@ public function testPurgeLogDataConcurrency()
{
$rawLogDao = new DataPurgingTest_RawLogDao(new DimensionMetadataProvider());
$rawLogDao->insertActionsOlderThanCallback = array($this, 'addReferenceToUnusedAction');
$purger = new LogDataPurger(new LogDeleter($rawLogDao), $rawLogDao);
$purger = new LogDataPurger(new LogDeleter($rawLogDao, new LogTablesProvider()), $rawLogDao);

$this->unusedIdAction = Db::fetchOne(
"SELECT idaction FROM " . Common::prefixTable('log_action') . " WHERE name = ?",
Expand Down
32 changes: 2 additions & 30 deletions tests/PHPUnit/Integration/LogDeleterTest.php
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
use Piwik\DataAccess\RawLogDao;
use Piwik\Db;
use Piwik\LogDeleter;
use Piwik\Tests\Framework\Mock\Plugin\LogTablesProvider;
use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
use Piwik\Tests\Framework\TestDataHelper\LogHelper;

Expand All @@ -34,7 +35,7 @@ public function setUp()
{
parent::setUp();

$this->logDeleter = new LogDeleter(new RawLogDao());
$this->logDeleter = new LogDeleter(new RawLogDao(), new LogTablesProvider());

$this->logInserter = new LogHelper();
$this->insertTestData();
Expand All @@ -50,35 +51,6 @@ public function test_deleteVisits_RemovesVisitsAndOtherRelatedLogs()
$this->assertVisitExists(4);
}

public function test_deleteConversions_RemovesConversionsAndConversionItems()
{
$this->logDeleter->deleteConversions(array(2, 3));

$this->assertConversionsNotExists(2);
$this->assertConversionsNotExists(3);

$this->assertVisitExists(1);
$this->assertVisitExists(2, $checkAggregates = false);
$this->assertVisitExists(3, $checkAggregates = false);
$this->assertVisitExists(4);
}

public function test_deleteConversionItems_RemovesConversionItems()
{
$this->logDeleter->deleteConversionItems(array(2, 3));

$this->assertConversionItemsNotExist(2);
$this->assertConversionItemsNotExist(3);

$this->assertConversionsExists(2, $checkAggregates = false);
$this->assertConversionsExists(3, $checkAggregates = false);

$this->assertVisitExists(1);
$this->assertVisitExists(2, $checkAggregates = false);
$this->assertVisitExists(3, $checkAggregates = false);
$this->assertVisitExists(4);
}

public function test_deleteVisitsFor_DeletesVisitsForSpecifiedRangeAndSites_AndInvokesCallbackAfterEveryChunkIsDeleted()
{
$iterationCount = 0;
Expand Down