From cf0957451ede30ccbf3899b3b8f5940c437fb897 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Mar 2016 06:39:26 -0800 Subject: [PATCH] Slightly simplify the Harbormaster build log API Summary: Ref T5822. This prepares for inline compression and garbage collection of build logs. This reduces the API surface area and removes a log from the "wait" step that would just log a message every 15 seconds. (If this is actually useful, I think we find a better way to communicate it.) Test Plan: Ran a build, saw a log: {F1136691} Reviewers: chad Reviewed By: chad Maniphest Tasks: T5822 Differential Revision: https://secure.phabricator.com/D15371 --- ...WaitForPreviousBuildStepImplementation.php | 17 ----- .../storage/build/HarbormasterBuild.php | 17 ----- .../storage/build/HarbormasterBuildLog.php | 73 ++++++++++--------- .../storage/build/HarbormasterBuildTarget.php | 5 +- .../worker/HarbormasterTargetWorker.php | 9 +-- 5 files changed, 42 insertions(+), 79 deletions(-) diff --git a/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php index 7f63eda072..eadf4c94f5 100644 --- a/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php @@ -31,24 +31,7 @@ final class HarbormasterWaitForPreviousBuildStepImplementation // Block until all previous builds of the same build plan have // finished. $plan = $build->getBuildPlan(); - - $existing_logs = id(new HarbormasterBuildLogQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withBuildTargetPHIDs(array($build_target->getPHID())) - ->execute(); - - if ($existing_logs) { - $log = head($existing_logs); - } else { - $log = $build->createLog($build_target, 'waiting', 'blockers'); - } - $blockers = $this->getBlockers($object, $plan, $build); - if ($blockers) { - $log->start(); - $log->append(pht("Blocked by: %s\n", implode(',', $blockers))); - $log->finalize(); - } if ($blockers) { throw new PhabricatorWorkerYieldException(15); diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 5134d0efa0..4d9278c7bf 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -234,23 +234,6 @@ final class HarbormasterBuild extends HarbormasterDAO return ($this->getPlanAutoKey() !== null); } - public function createLog( - HarbormasterBuildTarget $build_target, - $log_source, - $log_type) { - - $log_source = id(new PhutilUTF8StringTruncator()) - ->setMaximumBytes(250) - ->truncateString($log_source); - - $log = HarbormasterBuildLog::initializeNewBuildLog($build_target) - ->setLogSource($log_source) - ->setLogType($log_type) - ->save(); - - return $log; - } - public function retrieveVariablesFromBuild() { $results = array( 'buildable.diff' => null, diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 85e7ae2411..8b7c5c85bb 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -1,6 +1,7 @@ start) { - $this->finalize($this->start); + if ($this->getLive()) { + $this->closeBuildLog(); } } @@ -34,6 +34,35 @@ final class HarbormasterBuildLog extends HarbormasterDAO ->setLive(0); } + public function openBuildLog() { + if ($this->getLive()) { + throw new Exception(pht('This build log is already open!')); + } + + return $this + ->setLive(1) + ->save(); + } + + public function closeBuildLog() { + if (!$this->getLive()) { + throw new Exception(pht('This build log is not open!')); + } + + // TODO: Encode the log contents in a gzipped format. + + $this->reload(); + + $start = $this->getDateCreated(); + $now = PhabricatorTime::getNow(); + + return $this + ->setDuration($now - $start) + ->setLive(0) + ->save(); + } + + protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -73,26 +102,13 @@ final class HarbormasterBuildLog extends HarbormasterDAO return pht('Build Log'); } - public function start() { - if ($this->getLive()) { - throw new Exception( - pht('Live logging has already started for this log.')); - } - - $this->setLive(1); - $this->save(); - - $this->start = PhabricatorTime::getNow(); - - return time(); - } - public function append($content) { if (!$this->getLive()) { - throw new Exception( - pht('Start logging before appending data to the log.')); + throw new PhutilInvalidStateException('openBuildLog'); } - if (strlen($content) === 0) { + + $content = (string)$content; + if (!strlen($content)) { return; } @@ -152,21 +168,6 @@ final class HarbormasterBuildLog extends HarbormasterDAO } } - public function finalize($start = 0) { - if (!$this->getLive()) { - // TODO: Clean up this API. - return; - } - - // TODO: Encode the log contents in a gzipped format. - $this->reload(); - if ($start > 0) { - $this->setDuration(time() - $start); - } - $this->setLive(0); - $this->save(); - } - public function getLogText() { // TODO: This won't cope very well if we're pulling like a 700MB // log file out of the DB. We should probably implement some sort diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php index 27655189e6..8b47bdfc21 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php @@ -256,9 +256,8 @@ final class HarbormasterBuildTarget extends HarbormasterDAO $log = HarbormasterBuildLog::initializeNewBuildLog($this) ->setLogSource($log_source) - ->setLogType($log_type); - - $log->start(); + ->setLogType($log_type) + ->openBuildLog(); return $log; } diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php index ac3014dc29..fa3a01272b 100644 --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -90,13 +90,10 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { $target->setDateCompleted(PhabricatorTime::getNow()); $target->save(); } catch (Exception $ex) { - phlog($ex); - try { - $log = $build->createLog($target, 'core', 'exception'); - $start = $log->start(); - $log->append((string)$ex); - $log->finalize($start); + $log = $target->newLog('core', 'exception') + ->append($ex) + ->closeBuildLog(); } catch (Exception $log_ex) { phlog($log_ex); }