From 5b463e634ccf0f1b1e605e6c110e2341c62cae41 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Jan 2012 11:53:39 -0800 Subject: [PATCH] Write fewer "applied" rows and clean up excess historical rows Summary: - Only write the row if the rule is a one-time rule. - Delete all the rows for rules which aren't one-time. NOTE: This is probably like several million rows for Facebook and could take a while. Test Plan: Added some one-time and every-time rules, ran them against objects, verified only relevant rows were inserted. Ran upgrade script against a database with one-time and every-time "ruleapplied" rows, got the irrelevant rows removed. Reviewers: nh, btrahan, jungejason Reviewed By: btrahan CC: aran, epriestley Differential Revision: https://secure.phabricator.com/D1484 --- resources/sql/patches/102.heraldcleanup.php | 51 +++++++++++++++++++ .../test/HeraldTestConsoleController.php | 2 +- .../herald/engine/engine/HeraldEngine.php | 44 ++++++++++++++-- .../herald/engine/engine/__init__.php | 2 + .../herald/storage/rule/HeraldRule.php | 9 ---- ...habricatorRepositoryCommitHeraldWorker.php | 2 +- 6 files changed, 96 insertions(+), 14 deletions(-) create mode 100644 resources/sql/patches/102.heraldcleanup.php diff --git a/resources/sql/patches/102.heraldcleanup.php b/resources/sql/patches/102.heraldcleanup.php new file mode 100644 index 0000000000..64b57a3e14 --- /dev/null +++ b/resources/sql/patches/102.heraldcleanup.php @@ -0,0 +1,51 @@ +loadAll(); +foreach ($rules as $key => $rule) { + $first_policy = HeraldRepetitionPolicyConfig::toInt( + HeraldRepetitionPolicyConfig::FIRST); + if ($rule->getRepetitionPolicy() != $first_policy) { + unset($rules[$key]); + } +} + +$conn_w = id(new HeraldRule())->establishConnection('w'); + +$clause = ''; +if ($rules) { + $clause = qsprintf( + $conn_w, + 'WHERE ruleID NOT IN (%Ld)', + mpull($rules, 'getID')); +} + +echo "This may take a moment"; +do { + queryfx( + $conn_w, + 'DELETE FROM %T %Q LIMIT 1000', + HeraldRule::TABLE_RULE_APPLIED, + $clause); + echo "."; +} while ($conn_w->getAffectedRows()); + +echo "\n"; +echo "Done.\n"; diff --git a/src/applications/herald/controller/test/HeraldTestConsoleController.php b/src/applications/herald/controller/test/HeraldTestConsoleController.php index da94aa46d9..fa909d796e 100644 --- a/src/applications/herald/controller/test/HeraldTestConsoleController.php +++ b/src/applications/herald/controller/test/HeraldTestConsoleController.php @@ -96,7 +96,7 @@ class HeraldTestConsoleController extends HeraldController { $effects = $engine->applyRules($rules, $adapter); $dry_run = new HeraldDryRunAdapter(); - $engine->applyEffects($effects, $dry_run); + $engine->applyEffects($effects, $dry_run, $rules); $xscript = $engine->getTranscript(); diff --git a/src/applications/herald/engine/engine/HeraldEngine.php b/src/applications/herald/engine/engine/HeraldEngine.php index 9426d8afd1..b704e78542 100644 --- a/src/applications/herald/engine/engine/HeraldEngine.php +++ b/src/applications/herald/engine/engine/HeraldEngine.php @@ -34,7 +34,7 @@ class HeraldEngine { $engine = new HeraldEngine(); $effects = $engine->applyRules($rules, $object); - $engine->applyEffects($effects, $object); + $engine->applyEffects($effects, $object, $rules); return $engine->getTranscript(); } @@ -118,7 +118,11 @@ class HeraldEngine { return $effects; } - public function applyEffects(array $effects, HeraldObjectAdapter $object) { + public function applyEffects( + array $effects, + HeraldObjectAdapter $object, + array $rules) { + $this->transcript->setDryRun($object instanceof HeraldDryRunAdapter); $xscripts = $object->applyHeraldEffects($effects); @@ -132,18 +136,52 @@ class HeraldEngine { } if (!$this->transcript->getDryRun()) { + + $rules = mpull($rules, null, 'getID'); + $applied_ids = array(); + $first_policy = HeraldRepetitionPolicyConfig::toInt( + HeraldRepetitionPolicyConfig::FIRST); + // Mark all the rules that have had their effects applied as having been // executed for the current object. $rule_ids = mpull($xscripts, 'getRuleID'); + foreach ($rule_ids as $rule_id) { if (!$rule_id) { // Some apply transcripts are purely informational and not associated // with a rule, e.g. carryover emails from earlier revisions. continue; } - HeraldRule::saveRuleApplied($rule_id, $object->getPHID()); + + $rule = idx($rules, $rule_id); + if (!$rule) { + continue; + } + + if ($rule->getRepetitionPolicy() == $first_policy) { + $applied_ids[] = $rule_id; + } + } + + if ($applied_ids) { + $conn_w = id(new HeraldRule())->establishConnection('w'); + $sql = array(); + foreach ($applied_ids as $id) { + $sql[] = qsprintf( + $conn_w, + '(%s, %d)', + $object->getPHID(), + $id); + } + queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q', + HeraldRule::TABLE_RULE_APPLIED, + implode(', ', $sql)); } } + + die("DERP"); } public function getTranscript() { diff --git a/src/applications/herald/engine/engine/__init__.php b/src/applications/herald/engine/engine/__init__.php index 3898e3bb64..36f550d750 100644 --- a/src/applications/herald/engine/engine/__init__.php +++ b/src/applications/herald/engine/engine/__init__.php @@ -16,6 +16,8 @@ phutil_require_module('phabricator', 'applications/herald/storage/transcript/bas phutil_require_module('phabricator', 'applications/herald/storage/transcript/condition'); phutil_require_module('phabricator', 'applications/herald/storage/transcript/object'); phutil_require_module('phabricator', 'applications/herald/storage/transcript/rule'); +phutil_require_module('phabricator', 'storage/qsprintf'); +phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/herald/storage/rule/HeraldRule.php b/src/applications/herald/storage/rule/HeraldRule.php index 4938fc318c..c446c9db66 100644 --- a/src/applications/herald/storage/rule/HeraldRule.php +++ b/src/applications/herald/storage/rule/HeraldRule.php @@ -87,15 +87,6 @@ class HeraldRule extends HeraldDAO { return $this; } - public static function saveRuleApplied($rule_id, $phid) { - queryfx( - id(new HeraldRule())->establishConnection('w'), - 'INSERT IGNORE INTO %T (phid, ruleID) VALUES (%s, %d)', - self::TABLE_RULE_APPLIED, - $phid, - $rule_id); - } - public function loadConditions() { if (!$this->getID()) { return array(); diff --git a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php index acf34dfa68..49d3c0116f 100644 --- a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php @@ -42,7 +42,7 @@ class PhabricatorRepositoryCommitHeraldWorker $engine = new HeraldEngine(); $effects = $engine->applyRules($rules, $adapter); - $engine->applyEffects($effects, $adapter); + $engine->applyEffects($effects, $adapter, $rules); $email_phids = $adapter->getEmailPHIDs(); if (!$email_phids) {