From f225a6c51a80f817feac3540c33f16b43cdd3a0e Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sun, 17 Mar 2019 19:04:42 -0400 Subject: [PATCH 1/5] Reformat code - Reformat Console\Storage - Reformat Core\StorageManager - Simplify logic in Worker\CronJobs --- src/Core/Console/Storage.php | 30 ++++++++-------- src/Core/StorageManager.php | 2 +- src/Worker/CronJobs.php | 68 +++++++++++++++--------------------- 3 files changed, 44 insertions(+), 56 deletions(-) diff --git a/src/Core/Console/Storage.php b/src/Core/Console/Storage.php index 52f453471..a7a0ffd38 100644 --- a/src/Core/Console/Storage.php +++ b/src/Core/Console/Storage.php @@ -49,36 +49,36 @@ HELP; return -1; } - switch($this->args[0]) { - case 'list': - return $this->do_list(); - break; - case 'set': - return $this->do_set(); - break; - case 'move': - return $this->do_move(); - break; + switch ($this->args[0]) { + case 'list': + return $this->doList(); + break; + case 'set': + return $this->doSet(); + break; + case 'move': + return $this->doMove(); + break; } $this->out(sprintf('Invalid action "%s"', $this->args[0])); return -1; } - protected function do_list() + protected function doList() { $rowfmt = ' %-3s | %-20s'; $current = StorageManager::getBackend(); $this->out(sprintf($rowfmt, 'Sel', 'Name')); $this->out('-----------------------'); $isregisterd = false; - foreach(StorageManager::listBackends() as $name => $class) { + foreach (StorageManager::listBackends() as $name => $class) { $issel = ' '; if ($current === $class) { $issel = '*'; $isregisterd = true; }; - $this->out(sprintf($rowfmt, $issel , $name )); + $this->out(sprintf($rowfmt, $issel, $name)); } if ($current === '') { @@ -92,7 +92,7 @@ HELP; return 0; } - protected function do_set() + protected function doSet() { if (count($this->args) !== 2) { throw new CommandArgsException('Invalid arguments'); @@ -110,7 +110,7 @@ HELP; return 0; } - protected function do_move() + protected function doMove() { $tables = null; if (count($this->args) < 1 || count($this->args) > 2) { diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index ef6cef480..335c28f05 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -23,7 +23,7 @@ class StorageManager private static function setup() { - if (count(self::$backends)==0) { + if (count(self::$backends) == 0) { self::$backends = Config::get('storage', 'backends', self::$default_backends); } } diff --git a/src/Worker/CronJobs.php b/src/Worker/CronJobs.php index f723cf69f..5ebe91cf4 100644 --- a/src/Worker/CronJobs.php +++ b/src/Worker/CronJobs.php @@ -35,54 +35,42 @@ class CronJobs Logger::log("Starting cronjob " . $command, Logger::DEBUG); - // Call possible post update functions - // see src/Database/PostUpdate.php for more details - if ($command == 'post_update') { - PostUpdate::update(); - return; - } + switch($command) { + case 'post_update': + PostUpdate::update(); + break; - // update nodeinfo data - if ($command == 'nodeinfo') { - nodeinfo_cron(); - return; - } + case 'nodeinfo': + nodeinfo_cron(); + break; - // Expire and remove user entries - if ($command == 'expire_and_remove_users') { - self::expireAndRemoveUsers(); - return; - } + case 'expire_and_remove_users': + self::expireAndRemoveUsers(); + break; - if ($command == 'update_contact_birthdays') { - Contact::updateBirthdays(); - return; - } + case 'update_contact_birthdays': + Contact::updateBirthdays(); + break; - if ($command == 'update_photo_albums') { - self::updatePhotoAlbums(); - return; - } + case 'update_photo_albums': + self::updatePhotoAlbums(); + break; - // Clear cache entries - if ($command == 'clear_cache') { - self::clearCache($a); - return; - } + case 'clear_cache': + self::clearCache($a); + break; - // Repair missing Diaspora values in contacts - if ($command == 'repair_diaspora') { - self::repairDiaspora($a); - return; - } + case 'repair_diaspora': + self::repairDiaspora($a); + break; - // Repair entries in the database - if ($command == 'repair_database') { - self::repairDatabase(); - return; - } + case 'repair_database': + self::repairDatabase(); + break; - Logger::log("Xronjob " . $command . " is unknown.", Logger::DEBUG); + default: + Logger::log("Xronjob " . $command . " is unknown.", Logger::DEBUG); + } return; } From 7e2e2f425e7077cf62e214997c8a3fe3c1cb0106 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sun, 17 Mar 2019 19:12:20 -0400 Subject: [PATCH 2/5] Check that provided class implements IStorage in StorageManager::setBackend - Add notice in admin if setting value change failed - Add notice in console if setting value change failed --- mod/admin.php | 4 +++- src/Core/Console/Storage.php | 6 +++++- src/Core/StorageManager.php | 8 +++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/mod/admin.php b/mod/admin.php index 39c2e9341..f8a75b7a2 100644 --- a/mod/admin.php +++ b/mod/admin.php @@ -1200,7 +1200,9 @@ function admin_page_site_post(App $a) * @var $storagebackend \Friendica\Model\Storage\IStorage */ $storagebackend = Strings::escapeTags(trim(defaults($_POST, 'storagebackend', ''))); - StorageManager::setBackend($storagebackend); + if (!StorageManager::setBackend($storagebackend)) { + info(L10n::t('Invalid storage backend setting value.')); + } // save storage backend form if (!is_null($storagebackend) && $storagebackend != "") { diff --git a/src/Core/Console/Storage.php b/src/Core/Console/Storage.php index a7a0ffd38..8b02d874c 100644 --- a/src/Core/Console/Storage.php +++ b/src/Core/Console/Storage.php @@ -106,7 +106,11 @@ HELP; return -1; } - StorageManager::setBackend($class); + if (!StorageManager::setBackend($class)) { + $this->out($class . ' is not a valid backend storage class.'); + return -1; + } + return 0; } diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 335c28f05..3bb1cc451 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -54,12 +54,18 @@ class StorageManager * @brief Set current storage backend class * * @param string $class Backend class name + * @return bool * @throws \Friendica\Network\HTTPException\InternalServerErrorException */ public static function setBackend($class) { - /// @todo Check that $class implements IStorage + if (!in_array('Friendica\Model\Storage\IStorage', class_implements($class))) { + return false; + } + Config::set('storage', 'class', $class); + + return true; } /** From 8a48ff1f95f4063e975cf1ef70f07f8be0da7920 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Wed, 20 Mar 2019 00:33:26 -0400 Subject: [PATCH 3/5] Relax type constraint on limit parameter in Database\DBA --- src/Database/DBA.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Database/DBA.php b/src/Database/DBA.php index 832f0a444..5211c0910 100644 --- a/src/Database/DBA.php +++ b/src/Database/DBA.php @@ -1484,7 +1484,7 @@ class DBA } $limit_string = ''; - if (isset($params['limit']) && is_int($params['limit'])) { + if (isset($params['limit']) && is_numeric($params['limit'])) { $limit_string = " LIMIT " . intval($params['limit']); } From 8ddbeb087fe5defbbc9e72b969f61bd46f28f833 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Wed, 20 Mar 2019 00:41:57 -0400 Subject: [PATCH 4/5] Add limit parameter to storage move query - Fixes out-of-memory errors with large tables - Add database statement closing - Add meaningful variable names - Remove useless DBA::isResult check --- src/Core/StorageManager.php | 69 ++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index 3bb1cc451..0a8b35ce2 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -111,20 +111,20 @@ class StorageManager /** - * @brief Move resources to storage $dest + * @brief Move up to 5000 resources to storage $dest * * Copy existing data to destination storage and delete from source. * This method cannot move to legacy in-table `data` field. * - * @param string $dest Destination storage class name - * @param array|null $tables Tables to look in for resources. Optional, defaults to ['photo', 'attach'] - * - * @throws \Exception + * @param string $destination Storage class name + * @param array|null $tables Tables to look in for resources. Optional, defaults to ['photo', 'attach'] + * @param int $limit Limit of the process batch size, defaults to 5000 * @return int Number of moved resources + * @throws \Exception */ - public static function move($dest, $tables = null) + public static function move($destination, $tables = null, $limit = 5000) { - if (is_null($dest) || empty($dest)) { + if (empty($destination)) { throw new \Exception('Can\'t move to NULL storage backend'); } @@ -135,43 +135,42 @@ class StorageManager $moved = 0; foreach ($tables as $table) { // Get the rows where backend class is not the destination backend class - $rr = DBA::select( + $resources = DBA::select( $table, ['id', 'data', 'backend-class', 'backend-ref'], - ['`backend-class` IS NULL or `backend-class` != ?' , $dest ] + ['`backend-class` IS NULL or `backend-class` != ?', $destination], + ['limit' => $limit] ); - if (DBA::isResult($rr)) { - while($r = DBA::fetch($rr)) { - $id = $r['id']; - $data = $r['data']; - /** @var IStorage $backendClass */ - $backendClass = $r['backend-class']; - $backendRef = $r['backend-ref']; - if (!is_null($backendClass) && $backendClass !== '') { - Logger::log("get data from old backend " . $backendClass . " : " . $backendRef); - $data = $backendClass::get($backendRef); - } - - Logger::log("save data to new backend " . $dest); - /** @var IStorage $dest */ - $ref = $dest::put($data); - Logger::log("saved data as " . $ref); + while ($resource = DBA::fetch($resources)) { + $id = $resource['id']; + $data = $resource['data']; + /** @var IStorage $backendClass */ + $backendClass = $resource['backend-class']; + $backendRef = $resource['backend-ref']; + if (!empty($backendClass)) { + Logger::log("get data from old backend " . $backendClass . " : " . $backendRef); + $data = $backendClass::get($backendRef); + } - if ($ref !== '') { - Logger::log("update row"); - $ru = DBA::update($table, ['backend-class' => $dest, 'backend-ref' => $ref, 'data' => ''], ['id' => $id]); - - if ($ru) { - if (!is_null($backendClass) && $backendClass !== '') { - Logger::log("delete data from old backend " . $backendClass . " : " . $backendRef); - $backendClass::delete($backendRef); - } - $moved++; + Logger::log("save data to new backend " . $destination); + /** @var IStorage $destination */ + $ref = $destination::put($data); + Logger::log("saved data as " . $ref); + + if ($ref !== '') { + Logger::log("update row"); + if (DBA::update($table, ['backend-class' => $destination, 'backend-ref' => $ref, 'data' => ''], ['id' => $id])) { + if (!empty($backendClass)) { + Logger::log("delete data from old backend " . $backendClass . " : " . $backendRef); + $backendClass::delete($backendRef); } + $moved++; } } } + + DBA::close($resources); } return $moved; From 385e3a09f25be6d2ad8f0e43467d74121564dce7 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Wed, 20 Mar 2019 00:42:50 -0400 Subject: [PATCH 5/5] Add loop to console storage move - Add timestamp to output messages --- src/Core/Console/Storage.php | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/Core/Console/Storage.php b/src/Core/Console/Storage.php index 8b02d874c..ce89ce1e2 100644 --- a/src/Core/Console/Storage.php +++ b/src/Core/Console/Storage.php @@ -28,9 +28,10 @@ Synopsis Set current storage backend name storage backend to use. see "list". - bin/console storage move [table] + bin/console storage move [table] [-n 5000] Move stored data to current storage backend. table one of "photo" or "attach". default to both + -n limit of processed entry batch size HELP; return $help; } @@ -130,7 +131,17 @@ HELP; } $current = StorageManager::getBackend(); - $r = StorageManager::move($current, $tables); - $this->out(sprintf('Moved %d files', $r)); + $total = 0; + + do { + $moved = StorageManager::move($current, $tables, $this->getOption('n', 5000)); + if ($moved) { + $this->out(date('[Y-m-d H:i:s] ') . sprintf('Moved %d files', $moved)); + } + + $total += $moved; + } while ($moved); + + $this->out(sprintf(date('[Y-m-d H:i:s] ') . 'Moved %d files total', $total)); } }