From 251ee9b660b8987dfd10cbfae7ea9d9417e22466 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Mar 2017 11:31:57 -0700 Subject: [PATCH] Add dedicated "reviewers" storage to Differential and do double writes Summary: Ref T10967. This is an incremental step toward removing "reviewers" back to a dedicated storage table so we can handle changes like T11050. This adds the storage table, and starts doing double writes to it (so new or updated reviewers write to both the old edge table and the new "reviewers" table). Then we can do a migration, swap readers over one at a time, and eventually remove the old write and old storage and then implement new features. This change has no user-facing impact, it just causes us to write new data to two places instead of one. This is not completely exhaustive: the Herald "Add Reviewers" action is still doing a manual EDGE transaction. I'll clean that up next and do another pass to look for anything else I missed. This is also a bit copy/pastey for now but the logic around "RESIGN" is a little different in the two cases until T11050. I'll unify it in future changes. Test Plan: - Did a no-op edit. - Did a no-op comment. - Added reviewers. - Removed reviewers. - Accepted and rejected revisions. After all of these edits, did a `SELECT * FROM differential_reviewer` manually and saw consistent-looking rows in the database. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17495 --- .../sql/autopatches/20170313.reviewers.01.sql | 9 ++++ src/__phutil_library_map__.php | 2 + .../storage/DifferentialReviewer.php | 24 ++++++++++ .../DifferentialRevisionReviewTransaction.php | 39 +++++++++++++++ ...fferentialRevisionReviewersTransaction.php | 48 +++++++++++++++++++ 5 files changed, 122 insertions(+) create mode 100644 resources/sql/autopatches/20170313.reviewers.01.sql create mode 100644 src/applications/differential/storage/DifferentialReviewer.php diff --git a/resources/sql/autopatches/20170313.reviewers.01.sql b/resources/sql/autopatches/20170313.reviewers.01.sql new file mode 100644 index 0000000000..4b243b6f6f --- /dev/null +++ b/resources/sql/autopatches/20170313.reviewers.01.sql @@ -0,0 +1,9 @@ +CREATE TABLE {$NAMESPACE}_differential.differential_reviewer ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + revisionPHID VARBINARY(64) NOT NULL, + reviewerPHID VARBINARY(64) NOT NULL, + reviewerStatus VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_revision` (revisionPHID, reviewerPHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 544b4f2b39..1279ae4077 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -491,6 +491,7 @@ phutil_register_library_map(array( 'DifferentialRevertPlanCommitMessageField' => 'applications/differential/field/DifferentialRevertPlanCommitMessageField.php', 'DifferentialRevertPlanField' => 'applications/differential/customfield/DifferentialRevertPlanField.php', 'DifferentialReviewedByCommitMessageField' => 'applications/differential/field/DifferentialReviewedByCommitMessageField.php', + 'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php', 'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php', 'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php', 'DifferentialReviewerProxy' => 'applications/differential/storage/DifferentialReviewerProxy.php', @@ -5247,6 +5248,7 @@ phutil_register_library_map(array( 'DifferentialRevertPlanCommitMessageField' => 'DifferentialCommitMessageCustomField', 'DifferentialRevertPlanField' => 'DifferentialStoredCustomField', 'DifferentialReviewedByCommitMessageField' => 'DifferentialCommitMessageField', + 'DifferentialReviewer' => 'DifferentialDAO', 'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType', 'DifferentialReviewerProxy' => 'Phobject', diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php new file mode 100644 index 0000000000..72317a0a4c --- /dev/null +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -0,0 +1,24 @@ + array( + 'reviewerStatus' => 'text64', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_revision' => array( + 'columns' => array('revisionPHID', 'reviewerPHID'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + +} diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index 7e0c60a7b1..9ac731fea9 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -116,6 +116,9 @@ abstract class DifferentialRevisionReviewTransaction ); } + // This is currently double-writing: to the old (edge) store and the new + // (reviewer) store. Do the old edge write first. + $src_phid = $revision->getPHID(); $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; @@ -131,6 +134,42 @@ abstract class DifferentialRevisionReviewTransaction } $editor->save(); + + // Now, do the new write. + + if ($map) { + $table = new DifferentialReviewer(); + + $reviewers = $table->loadAllWhere( + 'revisionPHID = %s AND reviewerPHID IN (%Ls)', + $src_phid, + array_keys($map)); + $reviewers = mpull($reviewers, null, 'getReviewerPHID'); + + foreach ($map as $dst_phid => $edge_data) { + $reviewer = idx($reviewers, $dst_phid); + if (!$reviewer) { + $reviewer = id(new DifferentialReviewer()) + ->setRevisionPHID($src_phid) + ->setReviewerPHID($dst_phid); + } + + $reviewer->setReviewerStatus($status); + + if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { + if ($reviewer->getID()) { + $reviewer->delete(); + } + } else { + try { + $reviewer->save(); + } catch (AphrontDuplicateKeyQueryException $ex) { + // At least for now, just ignore it if we lost a race. + } + } + } + } + } } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php index 1e1b35c1c3..c4112a4516 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php @@ -106,6 +106,9 @@ final class DifferentialRevisionReviewersTransaction public function applyExternalEffects($object, $value) { $src_phid = $object->getPHID(); + // This is currently double-writing: to the old (edge) store and the new + // (reviewer) store. Do the old edge write first. + $old = $this->generateOldValue($object); $new = $value; $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; @@ -138,6 +141,51 @@ final class DifferentialRevisionReviewersTransaction } $editor->save(); + + // Now, do the new write. + + $table = new DifferentialReviewer(); + $table_name = $table->getTableName(); + $conn = $table->establishConnection('w'); + + if ($rem) { + queryfx( + $conn, + 'DELETE FROM %T WHERE revisionPHID = %s AND reviewerPHID IN (%Ls)', + $table_name, + $src_phid, + array_keys($rem)); + } + + if ($new) { + $reviewers = $table->loadAllWhere( + 'revisionPHID = %s AND reviewerPHID IN (%Ls)', + $src_phid, + array_keys($new)); + $reviewers = mpull($reviewers, null, 'getReviewerPHID'); + + foreach ($new as $dst_phid => $status) { + $old_status = idx($old, $dst_phid); + if ($old_status === $status) { + continue; + } + + $reviewer = idx($reviewers, $dst_phid); + if (!$reviewer) { + $reviewer = id(new DifferentialReviewer()) + ->setRevisionPHID($src_phid) + ->setReviewerPHID($dst_phid); + } + + $reviewer->setReviewerStatus($status); + + try { + $reviewer->save(); + } catch (AphrontDuplicateKeyQueryException $ex) { + // At least for now, just ignore it if we lost a race. + } + } + } } public function getTitle() {