Page MenuHomePhorge

ManiphestReportController: Separate legacy and synthetic data handling
ClosedPublic

Authored by aklapper on Oct 19 2024, 10:47.

Details

Summary

Due to code additions in rPcb957f8d and rPadbd7d4f required due to rPd321cc81, the code intertwines handling legacy data with handling/creating modern data.
Make things more understandable by clearly separating between both (handle one after the other) and by renaming some variables for clarity, so it will become slightly easier in the future to investigate this bottleneck (it is the only code querying the ManiphestTransaction table, leading to timeouts in large Phorge installations).

Also add a specific reference to the corresponding code change in a code comment, instead of a vague "late 2017".
Also, don't use the variable name $table for two different things (database vs AphrontTableView) in the same function.

Test Plan

Carefully read the code. Optionally, play with http://phorge.localhost/maniphest/report/burn/ with and without setting a project filter having tasks created in the codebase before 2017-11-22, and compare that the output is still the same.

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1604
Build 1604: arc lint + arc unit

Event Timeline

For context, the ManiphestTransaction table in my installation has >10mio entries so some charts do not render but time out. One approach could be Materialized Views but I don't see how that's feasible given the "Project" tag parameter coming into play (and having thousands of project tags in my installation), so my vague followup idea for my installation is to completely disable the code which handles the legacy data only (as creating the modern data does not rely on querying the ManiphestTransaction table), and thus deliberately ignore ancient legacy data from before 2018 for the sake of performance. That's easier to achieve when the upstream spaghetti code doesn't constantly switch between handling legacy and modern/synthetic data.

ARRRGH, I locally did this on master branch instead of a dedicated branch. Sorry, it's been a while!

20after4 subscribed.

Nice, it's a good step towards cleaning up this mess!

This revision is now accepted and ready to land.Oct 20 2024, 21:08
src/applications/maniphest/controller/ManiphestReportController.php
150

Maybe this ↑ should say "Phabricator", since Phorge was not existing in 2017

(sorry for my unuseful comment lol)

src/applications/maniphest/controller/ManiphestReportController.php
150

I edited the Wikipedia article (so it must be true!) to state that the first ancient relics of Phorge ever found were determined via C14 to be from the 18th century AD.

More relevant though: How can I move this very patchset into a branch on its own, sigh?

src/applications/maniphest/controller/ManiphestReportController.php
150

Uhm. Try running arc patch D25828 then arc diff again lol. I don't know :D Good luck

Try to make this a separate branch