Page MenuHomePhabricator

Migrate differential hunks to modern storage
AbandonedPublic

Authored by epriestley on Jun 9 2015, 8:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 10, 6:20 PM
Unknown Object (File)
Feb 22 2024, 2:20 PM
Unknown Object (File)
Feb 10 2024, 11:54 PM
Unknown Object (File)
Feb 9 2024, 6:18 AM
Unknown Object (File)
Jan 7 2024, 8:10 PM
Unknown Object (File)
Jan 7 2024, 8:06 PM
Unknown Object (File)
Jan 7 2024, 8:00 PM
Unknown Object (File)
Jan 3 2024, 7:01 PM

Details

Summary

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 https://secure.phabricator.com 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

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 6641
Build 6663: [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.
src/applications/differential/query/DifferentialHunkQuery.php
34–36

$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().