ManiphestReportController: Separate legacy and synthetic data handling
ClosedPublic
Actions

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

Details

Reviewers

20after4

Group Reviewers

O1: Blessed Committers

Commits

rPd643ca4c4c53: ManiphestReportController: Separate legacy and synthetic data handling

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

aklapper created this revision.Sat, Oct 19, 10:47

Owners added a reviewer: O1: Blessed Committers.Sat, Oct 19, 10:47

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSat, Oct 19, 10:47

aklapper requested review of this revision.Sat, Oct 19, 10:47

Harbormaster completed remote builds in B1604: Diff 2494.Sat, Oct 19, 10:47

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!

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

This revision is now accepted and ready to land.Sun, Oct 20, 21:08

valerio.bozzolan added inline comments.Mon, Oct 21, 05:30

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)

aklapper added inline comments.Mon, Oct 21, 08:32

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?