Page MenuHomePhabricator

Move Differential reviewers back to a dedicated database storage table
Closed, ResolvedPublic

Description

Long ago, Differential reviewers were moved from a dedicated table to the edge table. This now appears to have been a mistake.

Reviewers are significantly more complex than other edges. Reviewers are the only data type which uses edgedata, and we could remove this table and simplify the abstraction across all applications by moving reviewers back to a dedicated table.

Reviewers are complex enough to justify a runtime object (DifferentialReviewer) anyway, so we aren't meaningfully simplifying anything by using edges.

Technically, we do get slightly easier transaction rendering. But this is a tiny benefit in a vast gulf of costs and complexities.

A particularly nasty aspect of this is that a nontrivial amount of real transaction logic lives outside of the transaction editor. For example, DifferentialReviewersHeraldAction has logic to compare reviewer strength. It should not.

This isn't actively hemorrhaging or anything, it's just a fairly clear infrastructure defect.

Related:

  • (T10645) You can currently accept closed revisions, just not in the UI.
  • (T6040) Revision edits which upgrade or downgrade blocking reviewers are not made clear in the transaction log.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley renamed this task from Move Differential reviewers back to a dedicated table to Move Differential reviewers back to a dedicated database storage table.May 22 2016, 1:01 PM
eadler added a project: Restricted Project.Jun 14 2016, 10:19 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jun 16 2016, 6:28 PM

These pathways are still dodging ModularTransactions and thus not getting double writes after D17495:

  • The Herald action DifferentialReviewersHeraldAction does not yet use ModularTransactions, and does a direct edge write instead.
  • newAutoReviewTransaction(), which adds automatic reviewers from Owners, does a direct edge write.
  • Downgrading accepts and rejects (changing "Accepted" into "Accepted Older") uses raw edge transactions.
  • Marking reviewers as "commented" uses raw edge transactions. I'd like to make this orthogonal to primary status, though, and this isn't critical to retain through the double write phase, so I may just knock it out for now and restore it later.
  • Old-school resigns, accepts, and rejects are still using edges directly (can we get rid of these completely?).

These cases are OK:

  • Direct reviewer edits.
  • Modern accept/reject/resign.
  • The side effect from commandeering.

Old-school resigns, accepts, and rejects are still using edges directly (can we get rid of these completely?).

The blocker here is differential.createcomment. We could just remove it, but I believe it sees a meaningful amount of use (supporting ad-hoc unit test tie-ins from the era before Harbormaster, for example) and I think it's not too complex to keep it working without a compatibility break by silently upgrading accept, reject and resign into ModularTransaction transactions.

In theory we could upgrade everything but that might be a fair amount of work/testing and I think it is realistic to imagine that there are zero use cases for "resign-via-API" or "request-review-via-API" in the wild. Still, maybe this is easy enough to do that it's worthwhile to clean up now.

Beyond that, we still have some ACTION_CLOSE in commit parsing.

The Herald case and the next case (newAutoReviewTransaction()) both have some code duplication around prefiltering reviewers before building transactions for them.

There's at least some logic to this in Herald, since it allows us to generate a more accurate log message for users (i.e., "Herald added reviewers X and Y, but reviewer Z was already a reviewer"). For Owners, there's no direct utility.

I'm not cleaning this up for now because it's slightly tricky. In particular, when you "Edit Revision" and replace "@dog (blocking) with @dog", we want to downgrade the blocking reviewer to become a nonblocking reviewer. However, when a Herald rule or Owners rule adds @dog as a nonblocking reviewer, we don't want to downgrade.

We could add a key like ^ to the map (but: kind of hacky) to deal with this. Or we could add metadata to the transaction. But the extra logic isn't really hurting anything for now and this may be cleaner and easier after we swap storage, so I'm just leaving it untouched for the moment.

Downgrading accepts and rejects (changing "Accepted" into "Accepted Older") uses raw edge transactions.

I'm going to skip double writes here since I think this should become a synthetic state (i.e., state = accepted, diffPHID = some non-current diff) and I think we'll hit only a minor amount of temporary weirdness/ambiguity by not transitioning this directly.

Marking reviewers as "commented" uses raw edge transactions. I'd like to make this orthogonal to primary status, though, and this isn't critical to retain through the double write phase, so I may just knock it out for now and restore it later.

I'm going to skip double writes here too since I think this should also become a synthetic state (state = whatever, hasCommented = true).

I'll put hasCommented and diffPHID state into the new storage table instead.

epriestley added a comment.EditedMar 28 2017, 11:32 AM

From D17564, there's a minor workflow issue with "request review" incorrectly putting accepted changes back into an "accepted" state from this.

The workflow thing is somewhat nontrivial. Previously, we would explicitly downgrade "Accept" into "Accepted Older" when an author used "Request Review".

This was sort of a hack, where we treated the "Request Review" like an update without any text. We now represent "Accepted Older" as "Accepted" with a lastActionDiffPHID other than the current diff PHID, which is more accurate but means this hack doesn't work anymore.

The closest analog to the hack would be setting lastActionDiffPHID to a nonsense value, but I don't like this very much since it discards information.

A more authentic way to represent this state is probably to put a flag on each Reviewer like "the author has disavowed this review". We set the flag on all reviewers when we would have downgraded and clear it when a reviewer is touched by a review action. While the flag is set, a matching lastActionDiffPHID isn't good enough to count as a current "Accept".

epriestley closed this task as Resolved.Aug 15 2017, 5:51 PM
epriestley claimed this task.

We appear to have survived this. D18398 removed the double writes. There's a bit more cleanup (we could remove the edge type and actually destroy the older data) but that can happen whenever we get around to it.