Skip to content

Commit

Permalink
Do not retry failed indexing (#948)
Browse files Browse the repository at this point in the history
Signed-off-by: Varun Patil <[email protected]>
  • Loading branch information
pulsejet committed Mar 10, 2024
1 parent 730e523 commit ed53199
Show file tree
Hide file tree
Showing 12 changed files with 299 additions and 46 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file.
## [Unreleased]

- Hide files starting with `.` in the timeline
- Prevent automatically retrying failed indexing jobs

## [v6.2.2] - 2024-01-10

Expand Down
1 change: 1 addition & 0 deletions appinfo/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ function w($base, $param)
['name' => 'Admin#getSystemStatus', 'url' => '/api/system-status', 'verb' => 'GET'],
['name' => 'Admin#getSystemConfig', 'url' => '/api/system-config', 'verb' => 'GET'],
['name' => 'Admin#setSystemConfig', 'url' => '/api/system-config/{key}', 'verb' => 'PUT'],
['name' => 'Admin#getFailureLogs', 'url' => '/api/failure-logs', 'verb' => 'GET'],
['name' => 'Admin#placesSetup', 'url' => '/api/occ/places-setup', 'verb' => 'POST'],

// Service worker and assets
Expand Down
32 changes: 30 additions & 2 deletions lib/Command/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class IndexOpts
public ?string $user = null;
public ?string $folder = null;
public ?string $group = null;
public bool $retry = false;
public bool $skipCleanup = false;

public function __construct(InputInterface $input)
Expand All @@ -50,7 +51,8 @@ public function __construct(InputInterface $input)
$this->clear = (bool) $input->getOption('clear');
$this->user = $input->getOption('user');
$this->folder = $input->getOption('folder');
$this->skipCleanup = $input->getOption('skip-cleanup');
$this->retry = (bool) $input->getOption('retry');
$this->skipCleanup = (bool) $input->getOption('skip-cleanup');
$this->group = $input->getOption('group');
}
}
Expand Down Expand Up @@ -82,6 +84,7 @@ protected function configure(): void
->addOption('folder', null, InputOption::VALUE_REQUIRED, 'Index only the specified folder (relative to the user\'s root)')
->addOption('force', 'f', InputOption::VALUE_NONE, 'Force refresh of existing index entries')
->addOption('clear', null, InputOption::VALUE_NONE, 'Clear all existing index entries')
->addOption('retry', 'r', InputOption::VALUE_NONE, 'Retry indexing of failed files')
->addOption('skip-cleanup', null, InputOption::VALUE_NONE, 'Skip cleanup step (removing index entries with missing files)')
;
}
Expand Down Expand Up @@ -109,6 +112,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
// Perform steps based on opts
$this->checkClear();
$this->checkForce();
$this->checkRetry();

// Run the indexer
$this->runIndex();
Expand All @@ -118,6 +122,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$this->indexer->cleanupStale();
}

// Warn about skipped files
$this->warnRetry();

return 0;
} catch (\Exception $e) {
$this->output->writeln("<error>{$e->getMessage()}</error>".PHP_EOL);
Expand Down Expand Up @@ -161,10 +168,31 @@ protected function checkForce(): void
}

$this->output->writeln('Forcing refresh of existing index entries');

$this->tw->orphanAll();
}

/**
* Check and act on the retry option if set.
*/
protected function checkRetry(): void
{
if (!$this->opts->retry) {
return;
}

$this->output->writeln("<info>Retrying indexing of failed files</info>");
$this->tw->clearAllFailures();
}

/**
* Warn about skipped files (called at the end of indexing).
*/
protected function warnRetry(): void {
if ($count = $this->tw->countFailures()) {
$this->output->writeln("Indexing skipped for ${count} failed files, use --retry to try again");
}
}

/**
* Run the indexer.
*/
Expand Down
25 changes: 25 additions & 0 deletions lib/Controller/AdminController.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public function getSystemStatus(): Http\Response
return Util::guardEx(function () {
$config = \OC::$server->get(\OCP\IConfig::class);
$index = \OC::$server->get(\OCA\Memories\Service\Index::class);
$tw = \OC::$server->get(\OCA\Memories\Db\TimelineWrite::class);

// Build status array
$status = [];
Expand All @@ -107,6 +108,7 @@ public function getSystemStatus(): Http\Response

// Check number of indexed files
$status['indexed_count'] = $index->getIndexedCount();
$status['failure_count'] = $tw->countFailures();

// Automatic indexing stats
$jobStart = (int) $config->getAppValue(Application::APPNAME, 'last_index_job_start', (string) 0);
Expand Down Expand Up @@ -178,6 +180,29 @@ public function getSystemStatus(): Http\Response
});
}

/**
* @AdminRequired
*
* @NoCSRFRequired
*/
public function getFailureLogs(): Http\Response {
return Util::guardExDirect(static function (Http\IOutput $out) {
$tw = \OC::$server->get(\OCA\Memories\Db\TimelineWrite::class);

$out->setHeader('Content-Type: text/plain');
$out->setHeader('X-Accel-Buffering: no');
$out->setHeader('Cache-Control: no-cache');

foreach ($tw->listFailures() as $log) {
$fileid = str_pad((string) $log['fileid'], 12, ' ', STR_PAD_RIGHT); // size
$mtime = $log['mtime'];
$reason = $log['reason'];

$out->setOutput("{$fileid}[{$mtime}]\t{$reason}\n");
}
});
}

/**
* @AdminRequired
*
Expand Down
13 changes: 11 additions & 2 deletions lib/Db/TimelineWrite.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@
use OCP\IDBConnection;
use OCP\Lock\ILockingProvider;

const DELETE_TABLES = ['memories', 'memories_livephoto', 'memories_places'];
const DELETE_TABLES = ['memories', 'memories_livephoto', 'memories_places', 'memories_failures'];
const TRUNCATE_TABLES = ['memories_mapclusters'];

class TimelineWrite
{
use TimelineWriteMap;
use TimelineWriteOrphans;
use TimelineWritePlaces;
use TimelineWriteFailures;

public function __construct(
protected IDBConnection $connection,
Expand Down Expand Up @@ -182,7 +183,15 @@ public function processFile(
$query->insert('memories')->values($params);
}

return $query->executeStatement() > 0;
// Execute query
$updated = $query->executeStatement() > 0;

// Clear failures if successful
if ($updated) {
$this->clearFailures($file);
}

return $updated;
}

/**
Expand Down
93 changes: 93 additions & 0 deletions lib/Db/TimelineWriteFailures.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

declare(strict_types=1);

namespace OCA\Memories\Db;

use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\Files\File;
use OCP\IDBConnection;

trait TimelineWriteFailures
{
protected IDBConnection $connection;

/**
* Mark a file as failed indexing.
* The file will not be re-indexed until it changes.
*
* @param File $file The file that failed indexing
* @param string $reason The reason for the failure
*/
public function markFailed(File $file, string $reason): void
{
// Add file path to reason
$reason .= " ({$file->getPath()})";

// Remove all previous failures for this file
$this->connection->beginTransaction();
$this->clearFailures($file);

// Add the failure to the database
$query = $this->connection->getQueryBuilder();
$query->insert('memories_failures')
->values([
'fileid' => $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT),
'mtime' => $query->createNamedParameter($file->getMtime(), IQueryBuilder::PARAM_INT),
'reason' => $query->createNamedParameter($reason, IQueryBuilder::PARAM_STR),
])
->executeStatement()
;
$this->connection->commit();
}

/**
* Mark a file as successfully indexed.
* The entry will be removed from the failures table.
*
* @param File $file The file that was successfully indexed
*/
public function clearFailures(File $file): void
{
$query = $this->connection->getQueryBuilder();
$query->delete('memories_failures')
->where($query->expr()->eq('fileid', $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT)))
->executeStatement()
;
}

/**
* Get the count of failed files.
*/
public function countFailures(): int
{
$query = $this->connection->getQueryBuilder();
$query->select($query->createFunction('COUNT(fileid)'))
->from('memories_failures')
;
return (int) $query->executeQuery()->fetchOne();
}

/**
* Get the list of failures.
*/
public function listFailures(): array
{
return $this->connection->getQueryBuilder()
->select('*')
->from('memories_failures')
->executeQuery()
->fetchAll();
;
}

/**
* Clear all failures from the database.
*/
public function clearAllFailures(): void
{
// Delete all entries and reset autoincrement counter
$this->connection->executeStatement(
$this->connection->getDatabasePlatform()->getTruncateTableSQL('*PREFIX*memories_failures', false));
}
}
75 changes: 75 additions & 0 deletions lib/Migration/Version602003Date20240310203729.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Varun Patil <[email protected]>
* @author Varun Patil <[email protected]>
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

namespace OCA\Memories\Migration;

use OCP\DB\ISchemaWrapper;
use OCP\DB\Types;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version602003Date20240310203729 extends SimpleMigrationStep
{
/**
* @param Closure(): ISchemaWrapper $schemaClosure
*/
public function preSchemaChange(IOutput $output, \Closure $schemaClosure, array $options): void {}

/**
* @param \Closure(): ISchemaWrapper $schemaClosure
*/
public function changeSchema(IOutput $output, \Closure $schemaClosure, array $options): ?ISchemaWrapper
{
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

if (!$schema->hasTable('memories_failures')) {
$table = $schema->createTable('memories_failures');
$table->addColumn('id', 'integer', [
'autoincrement' => true,
'notnull' => true,
]);
$table->addColumn('fileid', Types::BIGINT, [
'notnull' => true,
'length' => 20,
]);
$table->addColumn('mtime', Types::BIGINT, [
'notnull' => true,
'length' => 20,
]);
$table->addColumn('reason', 'text', [
'notnull' => false,
]);

$table->setPrimaryKey(['id']);
$table->addIndex(['fileid', 'mtime'], 'memories_fail_fid_mt_idx');
}

return $schema;
}

/**
* @param Closure(): ISchemaWrapper $schemaClosure
*/
public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options): void {}
}
12 changes: 10 additions & 2 deletions lib/Service/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,17 +150,24 @@ public function indexFolder(Folder $folder): void
;

// Filter out files that are already indexed
$addFilter = static function (string $table, string $alias) use (&$query): void {
$addFilter = static function (
string $table,
string $alias,
bool $orphan = true,
) use (&$query): void {
$query->leftJoin('f', $table, $alias, $query->expr()->andX(
$query->expr()->eq('f.fileid', "{$alias}.fileid"),
$query->expr()->eq('f.mtime', "{$alias}.mtime"),
$query->expr()->eq("{$alias}.orphan", $query->expr()->literal(0)),
$orphan
? $query->expr()->eq("{$alias}.orphan", $query->expr()->literal(0))
: $query->expr()->literal(1),
));

$query->andWhere($query->expr()->isNull("{$alias}.fileid"));
};
$addFilter('memories', 'm');
$addFilter('memories_livephoto', 'lp');
$addFilter('memories_failures', 'fail', false);

// Get file IDs to actually index
$fileIds = $query->executeQuery()->fetchAll(\PDO::FETCH_COLUMN);
Expand Down Expand Up @@ -201,6 +208,7 @@ public function indexFile(File $file): void
$this->log("Skipping file {$path} due to lock", true);
} catch (\Exception $e) {
$this->error("Failed to index file {$path}: {$e->getMessage()}");
$this->tw->markFailed($file, $e->getMessage());
} finally {
$this->tempManager->clean();
}
Expand Down
1 change: 1 addition & 0 deletions src/components/admin/AdminTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export type ISystemStatus = {

bad_encryption: boolean;
indexed_count: number;
failure_count: number;
mimes: string[];
imagick: string | false;
gis_type: number;
Expand Down
Loading

0 comments on commit ed53199

Please sign in to comment.