Page MenuHomePhabricator

Regression: list of blocked/blocking tasks not shown for tasks with more than 100 related tasks
Closed, ResolvedPublic

Description

Issue:

Phabricator was upgraded on July 13th and that upgrade included the new "Task Graph" functionality. This graph functionality is unable to deal with very very large sets of dependent tasks (eg: many of our tracking tasks or other tasks with many sub-tasks or tasks being subtasks of tracking tasks). This initially prevented the task page from loading at all.

Temporary fix:

We have committed a temporary fix (authored by upstream) to disable the graph on tasks that have more than 100 related (sub- or parent) tasks.

Temporary workaround for lost functionality:

For tasks that the list of sub-tasks and parents tasks are not shown you can view them via "Edit related tasks" and selecting either "Edit Parent Tasks" or "Edit Subtasks" as appropriate. The full list will be displayed there and you can click on the icon to the left of the task title to open the task in a new browser tab.

Plan:

A real fix is in the works upstream to deal with this corner case more gracefully.

Event Timeline

Nemo_bis created this task.Jul 14 2016, 7:00 AM
Restricted Application added subscribers: Zppix, TerraCodes, Aklapper. · View Herald TranscriptJul 14 2016, 7:00 AM

See T140326#2460857 for some context., plus discussion in -devtools and -operations.

Thanks. Reverting to known-working Phabricator code would have been better, what now?

This is an upstream change and we will most likely wait for an upstream solution.

Restricted Application added a project: Upstream. · View Herald TranscriptJul 14 2016, 8:59 AM
greg renamed this task from Regression: list of blocked/blocking tasks disappeared to Regression: list of blocked/blocking tasks not shown for tasks with more than 100 subtasks.Jul 14 2016, 3:15 PM
greg added a subscriber: matmarex.

I can no longer see the subtasks of T107037 (without attempting to edit them). I think this is a major issue. It should at least fall back to a simple list of direct blocking/blocked tasks.

Nemo_bis renamed this task from Regression: list of blocked/blocking tasks not shown for tasks with more than 100 subtasks to Regression: list of blocked/blocking tasks not shown for tasks with more than 100 related tasks.Jul 14 2016, 5:06 PM

The summary change was incorrect; the count of 100 considers tasks other than subtasks, too.

Nemo_bis raised the priority of this task from Low to Needs Triage.Jul 14 2016, 5:09 PM
greg triaged this task as High priority.Jul 14 2016, 10:46 PM
greg updated the task description. (Show Details)
greg updated the task description. (Show Details)Jul 14 2016, 11:14 PM
greg updated the task description. (Show Details)
Danny_B updated the task description. (Show Details)Jul 14 2016, 11:22 PM

Perhaps I'm oversimplifying, but… can't we just do this?

diff --git a/src/utils/AbstractDirectedGraph.php b/src/utils/AbstractDirectedGraph.php
index 40242fa..5771827 100644
--- a/src/utils/AbstractDirectedGraph.php
+++ b/src/utils/AbstractDirectedGraph.php
@@ -218,6 +218,7 @@ abstract class AbstractDirectedGraph extends Phobject {
    * @task build
    */
   final public function loadGraph() {
+    $max_nodes = 100;
 
     $new_nodes = $this->knownNodes;
     while (true) {
@@ -230,11 +231,13 @@ abstract class AbstractDirectedGraph extends Phobject {
         }
       }
 
-      if (empty($load)) {
-        break;
-      }
-
+      $max_to_load = $max_nodes - count($this->knownNodes);
       $load = array_keys($load);
+      $load = array_slice($load, 0, $max_to_load);
+
+      if (empty($load)) {
+        break;
+      }
 
       $new_nodes = $this->loadEdges($load);
       foreach ($load as $node) {

This already loads the nodes in breadth-first order, so it should load the directly blocked/blocking tasks first, then blocked/blocking tasks of them, etc.

Fixed in https://phabricator.wikimedia.org/rPHABcf12fdf248df82dc414d96bddd147c058bc3d636 which has been deployed to phab-01.wmflabs.org

See task there at https://phab-01.wmflabs.org/T63 please.

If no issue arise it will deployed here soon.

mmodell closed this task as Resolved.Jul 15 2016, 10:35 AM
mmodell claimed this task.

Perhaps I'm oversimplifying, but… can't we just do this?

diff --git a/src/utils/AbstractDirectedGraph.php b/src/utils/AbstractDirectedGraph.php
index 40242fa..5771827 100644
--- a/src/utils/AbstractDirectedGraph.php
+++ b/src/utils/AbstractDirectedGraph.php
@@ -218,6 +218,7 @@ abstract class AbstractDirectedGraph extends Phobject {
    * @task build
    */
   final public function loadGraph() {
+    $max_nodes = 100;
     $new_nodes = $this->knownNodes;
     while (true) {
@@ -230,11 +231,13 @@ abstract class AbstractDirectedGraph extends Phobject {
         }
       }
-      if (empty($load)) {
-        break;
-      }
-
+      $max_to_load = $max_nodes - count($this->knownNodes);
       $load = array_keys($load);
+      $load = array_slice($load, 0, $max_to_load);
+
+      if (empty($load)) {
+        break;
+      }
       $new_nodes = $this->loadEdges($load);
       foreach ($load as $node) {

This already loads the nodes in breadth-first order, so it should load the directly blocked/blocking tasks first, then blocked/blocking tasks of them, etc.

@matmarex: What good does that do when there are more than 100 subtasks? There needs to be a fallback, apparently.

Ok, I now see the list of dependencies in the details at https://phabricator.wikimedia.org/T72163 and elsewhere. Thanks.