Move completed tasks to an "archive" table and delete them in the GC

epriestley · epriestley · commit 5903ed650c20 · 2012-10-31T15:22:16.000-07:00
Summary: Currently, when taskmasters complete a task it is immediately deleted. This prevents us from doing some general things, like: - Supporting the idea of permanent failure (e.g., after N failures just stop trying). - Showing the user how fast taskmasters are completing tasks. - Showing the user how long tasks took to complete. Having better visibility into this is important to Drydock, which builds on the task system. Also, generally buff debug output for task execution. Test Plan: Ran `bin/phd debug taskmaster`. Ran `bin/phd debug garbage`. Queued some tasks via various systems. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D3852
diff --git a/conf/default.conf.php b/conf/default.conf.php
@@ -1126,6 +1126,7 @@
   'gcdaemon.ttl.daemon-logs'                =>  7 * (24 * 60 * 60),
   'gcdaemon.ttl.differential-parse-cache'   => 14 * (24 * 60 * 60),
   'gcdaemon.ttl.markup-cache'               => 30 * (24 * 60 * 60),
+  'gcdaemon.ttl.task-archive'               => 14 * (24 * 60 * 60),
 
 
 // -- Feed ------------------------------------------------------------------ //
diff --git a/resources/sql/patches/daemontaskarchive.sql b/resources/sql/patches/daemontaskarchive.sql
@@ -0,0 +1,13 @@
+CREATE TABLE {$NAMESPACE}_worker.worker_archivetask (
+  id INT UNSIGNED PRIMARY KEY,
+  taskClass VARCHAR(255) NOT NULL COLLATE utf8_bin,
+  leaseOwner VARCHAR(255) COLLATE utf8_bin,
+  leaseExpires INT UNSIGNED,
+  failureCount INT UNSIGNED NOT NULL,
+  dataID INT UNSIGNED NOT NULL,
+  result INT UNSIGNED NOT NULL,
+  duration BIGINT UNSIGNED NOT NULL,
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL,
+  key(dateCreated)
+) ENGINE=InnoDB, COLLATE utf8_general_ci;
diff --git a/scripts/mail/create_worker_tasks.php b/scripts/mail/create_worker_tasks.php
@@ -33,10 +33,9 @@
 
 foreach ($messages as $message) {
   if (!$message->getWorkerTaskID()) {
-    $mailer_task = new PhabricatorWorkerTask();
-    $mailer_task->setTaskClass('PhabricatorMetaMTAWorker');
-    $mailer_task->setData($message->getID());
-    $mailer_task->save();
+    $mailer_task = PhabricatorWorker::scheduleTask(
+      'PhabricatorMetaMTAWorker',
+      $message->getID());
 
     $message->setWorkerTaskID($mailer_task->getID());
     $message->save();
diff --git a/scripts/repository/reparse.php b/scripts/repository/reparse.php
@@ -224,10 +224,7 @@
 
   if ($all_from_repo && !$force_local) {
     foreach ($classes as $class) {
-      $task = new PhabricatorWorkerTask();
-      $task->setTaskClass($class);
-      $task->setData($spec);
-      $task->save();
+      PhabricatorWorker::scheduleTask($class, $spec);
 
       $commit_name = 'r'.$callsign.$commit->getCommitIdentifier();
       echo "  Queued '{$class}' for commit '{$commit_name}'.\n";
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
@@ -1140,6 +1140,8 @@
     'PhabricatorUserStatusOverlapException' => 'applications/people/exception/PhabricatorUserStatusOverlapException.php',
     'PhabricatorUserTestCase' => 'applications/people/storage/__tests__/PhabricatorUserTestCase.php',
     'PhabricatorWorker' => 'infrastructure/daemon/workers/PhabricatorWorker.php',
+    'PhabricatorWorkerActiveTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php',
+    'PhabricatorWorkerArchiveTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php',
     'PhabricatorWorkerDAO' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerDAO.php',
     'PhabricatorWorkerTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php',
     'PhabricatorWorkerTaskData' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerTaskData.php',
@@ -2300,6 +2302,8 @@
     'PhabricatorUserStatusInvalidEpochException' => 'Exception',
     'PhabricatorUserStatusOverlapException' => 'Exception',
     'PhabricatorUserTestCase' => 'PhabricatorTestCase',
+    'PhabricatorWorkerActiveTask' => 'PhabricatorWorkerTask',
+    'PhabricatorWorkerArchiveTask' => 'PhabricatorWorkerTask',
     'PhabricatorWorkerDAO' => 'PhabricatorLiskDAO',
     'PhabricatorWorkerTask' => 'PhabricatorWorkerDAO',
     'PhabricatorWorkerTaskData' => 'PhabricatorWorkerDAO',
diff --git a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php
@@ -34,7 +34,7 @@ public function processRequest() {
     $daemon_panel->setHeader('Recently Launched Daemons');
     $daemon_panel->appendChild($daemon_table);
 
-    $tasks = id(new PhabricatorWorkerTask())->loadAllWhere(
+    $tasks = id(new PhabricatorWorkerActiveTask())->loadAllWhere(
       'leaseOwner IS NOT NULL');
 
     $rows = array();
@@ -80,7 +80,7 @@ public function processRequest() {
     $leased_panel->setHeader('Leased Tasks');
     $leased_panel->appendChild($leased_table);
 
-    $task_table = new PhabricatorWorkerTask();
+    $task_table = new PhabricatorWorkerActiveTask();
     $queued = queryfx_all(
       $task_table->establishConnection('r'),
       'SELECT taskClass, count(*) N FROM %T GROUP BY taskClass
diff --git a/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php b/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php
@@ -29,7 +29,7 @@ public function processRequest() {
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $task = id(new PhabricatorWorkerTask())->load($this->id);
+    $task = id(new PhabricatorWorkerActiveTask())->load($this->id);
     if (!$task) {
       $error_view = new AphrontErrorView();
       $error_view->setTitle('No Such Task');
diff --git a/src/applications/daemon/controller/PhabricatorWorkerTaskUpdateController.php b/src/applications/daemon/controller/PhabricatorWorkerTaskUpdateController.php
@@ -31,7 +31,7 @@ public function processRequest() {
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $task = id(new PhabricatorWorkerTask())->load($this->id);
+    $task = id(new PhabricatorWorkerActiveTask())->load($this->id);
     if (!$task) {
       return new Aphront404Response();
     }
diff --git a/src/applications/drydock/allocator/DrydockAllocator.php b/src/applications/drydock/allocator/DrydockAllocator.php
@@ -59,10 +59,7 @@ public function allocate() {
       $worker = new DrydockAllocatorWorker($data);
       $worker->executeTask();
     } else {
-      $task = new PhabricatorWorkerTask();
-      $task->setTaskClass('DrydockAllocatorWorker');
-      $task->setData($data);
-      $task->save();
+      PhabricatorWorker::scheduleTask('DrydockAllocatorWorker', $data);
     }
 
     return $lease;
diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
@@ -298,10 +298,10 @@ protected function didWriteData() {
     parent::didWriteData();
 
     if (!$this->getWorkerTaskID()) {
-      $mailer_task = new PhabricatorWorkerTask();
-      $mailer_task->setTaskClass('PhabricatorMetaMTAWorker');
-      $mailer_task->setData($this->getID());
-      $mailer_task->save();
+      $mailer_task = PhabricatorWorker::scheduleTask(
+        'PhabricatorMetaMTAWorker',
+        $this->getID());
+
       $this->setWorkerTaskID($mailer_task->getID());
       $this->save();
     }
diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
@@ -450,12 +450,9 @@ private function insertTask(
         throw new Exception("Unknown repository type '{$vcs}'!");
     }
 
-    $task = new PhabricatorWorkerTask();
-    $task->setTaskClass($class);
     $data['commitID'] = $commit->getID();
 
-    $task->setData($data);
-    $task->save();
+    PhabricatorWorker::scheduleTask($class, $data);
   }
 
 
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -73,13 +73,11 @@ protected function parseCommit(
     }
 
     if ($this->shouldQueueFollowupTasks()) {
-      $herald_task = new PhabricatorWorkerTask();
-      $herald_task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker');
-      $herald_task->setData(
+      PhabricatorWorker::scheduleTask(
+        'PhabricatorRepositoryCommitHeraldWorker',
         array(
           'commitID' => $commit->getID(),
         ));
-      $herald_task->save();
     }
   }
 
diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
@@ -76,13 +76,11 @@ protected function finishParse() {
     PhabricatorSearchCommitIndexer::indexCommit($commit);
 
     if ($this->shouldQueueFollowupTasks()) {
-      $owner_task = new PhabricatorWorkerTask();
-      $owner_task->setTaskClass('PhabricatorRepositoryCommitOwnersWorker');
-      $owner_task->setData(
+      PhabricatorWorker::scheduleTask(
+        'PhabricatorRepositoryCommitOwnersWorker',
         array(
           'commitID' => $commit->getID(),
         ));
-      $owner_task->save();
     }
   }
 
diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php
@@ -69,13 +69,11 @@ public function parseCommit(
     $this->updateCommitData($author, $message, $committer);
 
     if ($this->shouldQueueFollowupTasks()) {
-      $task = new PhabricatorWorkerTask();
-      $task->setTaskClass('PhabricatorRepositoryGitCommitChangeParserWorker');
-      $task->setData(
+      PhabricatorWorker::scheduleTask(
+        'PhabricatorRepositoryGitCommitChangeParserWorker',
         array(
           'commitID' => $commit->getID(),
         ));
-      $task->save();
     }
   }
 
diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php
@@ -37,14 +37,11 @@ public function parseCommit(
     $this->updateCommitData($author, $message);
 
     if ($this->shouldQueueFollowupTasks()) {
-      $task = new PhabricatorWorkerTask();
-      $task->setTaskClass(
-        'PhabricatorRepositoryMercurialCommitChangeParserWorker');
-      $task->setData(
+      PhabricatorWorker::scheduleTask(
+        'PhabricatorRepositoryMercurialCommitChangeParserWorker',
         array(
           'commitID' => $commit->getID(),
         ));
-      $task->save();
     }
   }
 
diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php
@@ -38,13 +38,11 @@ public function parseCommit(
     $this->updateCommitData($author, $message);
 
     if ($this->shouldQueueFollowupTasks()) {
-      $task = new PhabricatorWorkerTask();
-      $task->setTaskClass('PhabricatorRepositorySvnCommitChangeParserWorker');
-      $task->setData(
+      PhabricatorWorker::scheduleTask(
+        'PhabricatorRepositorySvnCommitChangeParserWorker',
         array(
           'commitID' => $commit->getID(),
         ));
-      $task->save();
     }
   }
 
diff --git a/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php b/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php
@@ -64,12 +64,14 @@ public function run() {
       $n_daemon = $this->collectDaemonLogs();
       $n_parse  = $this->collectParseCaches();
       $n_markup = $this->collectMarkupCaches();
+      $n_tasks  = $this->collectArchivedTasks();
 
       $collected = array(
         'Herald Transcript'           => $n_herald,
         'Daemon Log'                  => $n_daemon,
         'Differential Parse Cache'    => $n_parse,
         'Markup Cache'                => $n_markup,
+        'Archived Tasks'              => $n_tasks,
       );
       $collected = array_filter($collected);
 
@@ -172,4 +174,46 @@ private function collectMarkupCaches() {
     return $conn_w->getAffectedRows();
   }
 
+  private function collectArchivedTasks() {
+    $key = 'gcdaemon.ttl.task-archive';
+    $ttl = PhabricatorEnv::getEnvConfig($key);
+    if ($ttl <= 0) {
+      return 0;
+    }
+
+    $table = new PhabricatorWorkerArchiveTask();
+    $data_table = new PhabricatorWorkerTaskData();
+    $conn_w = $table->establishConnection('w');
+
+    $rows = queryfx_all(
+      $conn_w,
+      'SELECT id, dataID FROM %T WHERE dateCreated < %d LIMIT 100',
+      $table->getTableName(),
+      time() - $ttl);
+
+    if (!$rows) {
+      return 0;
+    }
+
+    $data_ids = array_filter(ipull($rows, 'dataID'));
+    $task_ids = ipull($rows, 'id');
+
+    $table->openTransaction();
+      if ($data_ids) {
+        queryfx(
+          $conn_w,
+          'DELETE FROM %T WHERE id IN (%Ld)',
+          $data_table->getTableName(),
+          $data_ids);
+      }
+      queryfx(
+        $conn_w,
+        'DELETE FROM %T WHERE id IN (%Ld)',
+        $table->getTableName(),
+        $task_ids);
+    $table->saveTransaction();
+
+    return count($task_ids);
+  }
+
 }
diff --git a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php
@@ -21,11 +21,13 @@ final class PhabricatorTaskmasterDaemon extends PhabricatorDaemon {
   public function run() {
     $lease_ownership_name = $this->getLeaseOwnershipName();
 
-    $task_table = new PhabricatorWorkerTask();
+    $task_table = new PhabricatorWorkerActiveTask();
     $taskdata_table = new PhabricatorWorkerTaskData();
 
     $sleep = 0;
     do {
+      $this->log('Dequeuing a task...');
+
       $conn_w = $task_table->establishConnection('w');
       queryfx(
         $conn_w,
@@ -36,6 +38,7 @@ public function run() {
       $rows = $conn_w->getAffectedRows();
 
       if (!$rows) {
+        $this->log('No unleased tasks. Dequeuing an expired lease...');
         queryfx(
           $conn_w,
           'UPDATE %T SET leaseOwner = %s, leaseExpires = UNIX_TIMESTAMP() + 15
@@ -70,14 +73,18 @@ public function run() {
         }
 
         foreach ($tasks as $task) {
+          $id = $task->getID();
+          $class = $task->getTaskClass();
+
+          $this->log("Working on task {$id} ({$class})...");
+
           // TODO: We should detect if we acquired a task with an expired lease
           // and log about it / bump up failure count.
 
           // TODO: We should detect if we acquired a task with an excessive
           // failure count and fail it permanently.
 
           $data = idx($task_data, $task->getID());
-          $class = $task->getTaskClass();
           try {
             if (!class_exists($class) ||
                 !is_subclass_of($class, 'PhabricatorWorker')) {
@@ -91,19 +98,19 @@ public function run() {
               $task->setLeaseDuration($lease);
             }
 
+            $t_start = microtime(true);
             $worker->executeTask();
+            $t_end = microtime(true);
 
-            $task->delete();
-            if ($data !== null) {
-              queryfx(
-                $conn_w,
-                'DELETE FROM %T WHERE id = %d',
-                $taskdata_table->getTableName(),
-                $task->getDataID());
-            }
+            $task->archiveTask(
+              PhabricatorWorkerArchiveTask::RESULT_SUCCESS,
+              (int)(1000000 * ($t_end - $t_start)));
+            $this->log("Task {$id} complete! Moved to archive.");
           } catch (Exception $ex) {
             $task->setFailureCount($task->getFailureCount() + 1);
             $task->save();
+
+            $this->log("Task {$id} failed!");
             throw $ex;
           }
         }
diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -37,4 +37,12 @@ public function getRequiredLeaseTime() {
   }
 
   abstract protected function doWork();
+
+  final public static function scheduleTask($task_class, $data) {
+    return id(new PhabricatorWorkerActiveTask())
+      ->setTaskClass($task_class)
+      ->setData($data)
+      ->save();
+  }
+
 }
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerDAO.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerDAO.php
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php
diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ public function processRequest() {`
`31`	`31`	`$request = $this->getRequest();`
`32`	`32`	`$user = $request->getUser();`
`33`	`33`
`34`		`- $task = id(new PhabricatorWorkerTask())->load($this->id);`
	`34`	`+ $task = id(new PhabricatorWorkerActiveTask())->load($this->id);`
`35`	`35`	`if (!$task) {`
`36`	`36`	`return new Aphront404Response();`
`37`	`37`	`}`