Page MenuHomePhabricator

Migrate Differential comments to ApplicationTransactions
ClosedPublic

Authored by epriestley on Feb 12 2014, 7:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 3:12 PM
Unknown Object (File)
Sat, Nov 23, 12:26 AM
Unknown Object (File)
Tue, Nov 19, 10:34 AM
Unknown Object (File)
Tue, Nov 19, 12:27 AM
Unknown Object (File)
Mon, Nov 18, 1:13 PM
Unknown Object (File)
Mon, Nov 18, 3:16 AM
Unknown Object (File)
Thu, Nov 14, 9:36 PM
Unknown Object (File)
Sun, Nov 10, 10:48 PM
Subscribers

Details

Summary

Ref T2222. This is the big one.

This migrates each DifferentialComment to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes DifferentialComment a double-reader for ApplicationTransactions.

The migration is pretty straightforward:

  • If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.".
  • If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook).
  • If a comment added or removed reviewers, it gets a "changed reviewers" transaction.
  • If a comment added CCs, it gets a "subscribers" transaction.
  • If a comment added comment text, it gets a "comment" transaction.
  • For each inline attached to a comment, we generate an "inline" transaction.

Most comments generate a small number of transactions, but a few generate a significant number.

At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines).

Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table.

NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code.

Specifically, they look like this:

{F112270}

Test Plan

I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place.

IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master.

I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made.

Diff Detail

Repository
rP Phabricator
Branch
dxn3
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/differential/query/DifferentialCommentQuery.php:17XHP16TODO Comment
Unit
Tests Passed

Event Timeline

src/applications/differential/editor/DifferentialCommentEditor.php
704

Oh, I need to deal with this too. I'm going to put that in a separate diff.

src/applications/differential/view/DifferentialRevisionCommentListView.php
96

This is like "TODO: Fix inline comment rendering by using ApplicationTransactions", more or less.

i spent extra time with this one. Branch strat sounds good.

Thanks! I'm going to get a diff or two ahead of this in my sandbox and see what else I can catch before I push it -- I caught one very small thing so far, but everything else has been looking good.

epriestley updated this revision to Unknown Object (????).Feb 12 2014, 10:07 PM

We no longer need to make a "comment" transaction if there are inlines, since we'll write an inline transaction for each of them.

This fixes an issue where leaving an empty comment, plus some inlines, would create an empty comment transaction. Now, we'll just create inline transactions instead.