@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
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
+122
+9
resources/sql/autopatches/20170313.reviewers.01.sql
+9
resources/sql/autopatches/20170313.reviewers.01.sql
···
1
+
CREATE TABLE {$NAMESPACE}_differential.differential_reviewer (
2
+
id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
3
+
revisionPHID VARBINARY(64) NOT NULL,
4
+
reviewerPHID VARBINARY(64) NOT NULL,
5
+
reviewerStatus VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT},
6
+
dateCreated INT UNSIGNED NOT NULL,
7
+
dateModified INT UNSIGNED NOT NULL,
8
+
UNIQUE KEY `key_revision` (revisionPHID, reviewerPHID)
9
+
) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
+2
src/__phutil_library_map__.php
+2
src/__phutil_library_map__.php
···
491
491
'DifferentialRevertPlanCommitMessageField' => 'applications/differential/field/DifferentialRevertPlanCommitMessageField.php',
492
492
'DifferentialRevertPlanField' => 'applications/differential/customfield/DifferentialRevertPlanField.php',
493
493
'DifferentialReviewedByCommitMessageField' => 'applications/differential/field/DifferentialReviewedByCommitMessageField.php',
494
+
'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php',
494
495
'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php',
495
496
'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php',
496
497
'DifferentialReviewerProxy' => 'applications/differential/storage/DifferentialReviewerProxy.php',
···
5247
5248
'DifferentialRevertPlanCommitMessageField' => 'DifferentialCommitMessageCustomField',
5248
5249
'DifferentialRevertPlanField' => 'DifferentialStoredCustomField',
5249
5250
'DifferentialReviewedByCommitMessageField' => 'DifferentialCommitMessageField',
5251
+
'DifferentialReviewer' => 'DifferentialDAO',
5250
5252
'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
5251
5253
'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType',
5252
5254
'DifferentialReviewerProxy' => 'Phobject',
+24
src/applications/differential/storage/DifferentialReviewer.php
+24
src/applications/differential/storage/DifferentialReviewer.php
···
1
+
<?php
2
+
3
+
final class DifferentialReviewer
4
+
extends DifferentialDAO {
5
+
6
+
protected $revisionPHID;
7
+
protected $reviewerPHID;
8
+
protected $reviewerStatus;
9
+
10
+
protected function getConfiguration() {
11
+
return array(
12
+
self::CONFIG_COLUMN_SCHEMA => array(
13
+
'reviewerStatus' => 'text64',
14
+
),
15
+
self::CONFIG_KEY_SCHEMA => array(
16
+
'key_revision' => array(
17
+
'columns' => array('revisionPHID', 'reviewerPHID'),
18
+
'unique' => true,
19
+
),
20
+
),
21
+
) + parent::getConfiguration();
22
+
}
23
+
24
+
}
+39
src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+39
src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
···
116
116
);
117
117
}
118
118
119
+
// This is currently double-writing: to the old (edge) store and the new
120
+
// (reviewer) store. Do the old edge write first.
121
+
119
122
$src_phid = $revision->getPHID();
120
123
$edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
121
124
···
131
134
}
132
135
133
136
$editor->save();
137
+
138
+
// Now, do the new write.
139
+
140
+
if ($map) {
141
+
$table = new DifferentialReviewer();
142
+
143
+
$reviewers = $table->loadAllWhere(
144
+
'revisionPHID = %s AND reviewerPHID IN (%Ls)',
145
+
$src_phid,
146
+
array_keys($map));
147
+
$reviewers = mpull($reviewers, null, 'getReviewerPHID');
148
+
149
+
foreach ($map as $dst_phid => $edge_data) {
150
+
$reviewer = idx($reviewers, $dst_phid);
151
+
if (!$reviewer) {
152
+
$reviewer = id(new DifferentialReviewer())
153
+
->setRevisionPHID($src_phid)
154
+
->setReviewerPHID($dst_phid);
155
+
}
156
+
157
+
$reviewer->setReviewerStatus($status);
158
+
159
+
if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) {
160
+
if ($reviewer->getID()) {
161
+
$reviewer->delete();
162
+
}
163
+
} else {
164
+
try {
165
+
$reviewer->save();
166
+
} catch (AphrontDuplicateKeyQueryException $ex) {
167
+
// At least for now, just ignore it if we lost a race.
168
+
}
169
+
}
170
+
}
171
+
}
172
+
134
173
}
135
174
136
175
}
+48
src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php
+48
src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php
···
106
106
public function applyExternalEffects($object, $value) {
107
107
$src_phid = $object->getPHID();
108
108
109
+
// This is currently double-writing: to the old (edge) store and the new
110
+
// (reviewer) store. Do the old edge write first.
111
+
109
112
$old = $this->generateOldValue($object);
110
113
$new = $value;
111
114
$edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
···
138
141
}
139
142
140
143
$editor->save();
144
+
145
+
// Now, do the new write.
146
+
147
+
$table = new DifferentialReviewer();
148
+
$table_name = $table->getTableName();
149
+
$conn = $table->establishConnection('w');
150
+
151
+
if ($rem) {
152
+
queryfx(
153
+
$conn,
154
+
'DELETE FROM %T WHERE revisionPHID = %s AND reviewerPHID IN (%Ls)',
155
+
$table_name,
156
+
$src_phid,
157
+
array_keys($rem));
158
+
}
159
+
160
+
if ($new) {
161
+
$reviewers = $table->loadAllWhere(
162
+
'revisionPHID = %s AND reviewerPHID IN (%Ls)',
163
+
$src_phid,
164
+
array_keys($new));
165
+
$reviewers = mpull($reviewers, null, 'getReviewerPHID');
166
+
167
+
foreach ($new as $dst_phid => $status) {
168
+
$old_status = idx($old, $dst_phid);
169
+
if ($old_status === $status) {
170
+
continue;
171
+
}
172
+
173
+
$reviewer = idx($reviewers, $dst_phid);
174
+
if (!$reviewer) {
175
+
$reviewer = id(new DifferentialReviewer())
176
+
->setRevisionPHID($src_phid)
177
+
->setReviewerPHID($dst_phid);
178
+
}
179
+
180
+
$reviewer->setReviewerStatus($status);
181
+
182
+
try {
183
+
$reviewer->save();
184
+
} catch (AphrontDuplicateKeyQueryException $ex) {
185
+
// At least for now, just ignore it if we lost a race.
186
+
}
187
+
}
188
+
}
141
189
}
142
190
143
191
public function getTitle() {