Page MenuHomePhabricator

Migrate differential hunks to modern storage

Authored by epriestley on Jun 9 2015, 8:30 AM.



Fixes T8475. Fixes T8623. A long time ago (in D9290), a new hunk storage was added. From this point, new Differential diffs were written to the "modern" storage, whilst existing diffs were kept in the "legacy" storage. A command line script (./bin/hunks migrate) was provided to migrate data from legacy storage to modern storage, but this script only migrates one-hunk-at-a-time. Given that this code has proven to be stable (all of the hunks on our install were migrated over 6 months ago, and @epriestley mentioned that did the same), we should force all installs to migrate hunks to modern storage.

Test Plan

Checked out the following versions of the code base:

Created a few diffs and then upgraded to HEAD. Applied migrations successfully and then browsed around migrated diffs in the UI.

Diff Detail

rP Phabricator
Lint OK
Unit Tests OK
Build Status
Buildable 6777
Build 19474: Run Core Tests
Build 6799: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Migrate differential hunks to modern storage.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
  • Remove done text
  • Remove inaccurate comment

This seems generally reasonable, but I want to give installs a little more warning:

  • I'll issue guidance in the next couple changelogs to do this yourself (it can be done while Phabricator is running, cancelled/resumed safely, etc), so it's better for large installs if they do this prior to a normal upgrade.
  • After the final warning, we can land this and force everyone else through it.

$all_results and comment both obsolete now?

joshuaspence marked an inline comment as done.
joshuaspence edited edge metadata.

Remove comments which are no longer relevant

@joshuaspence I'm curious about the status of this. It doesn't cleanly apply to HEAD but still seems like something that is rather useful to get done for both performance and code cleanlyness.

What are you curious about exactly?

I think it's probably safe to land this now?

epriestley edited reviewers, added: joshuaspence; removed: epriestley.

I'm going to split this up a bit and try to avoid calling save().