Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix metadata storage with sharding #48563

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apps/dav/lib/Connector/Sabre/FilesPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
namespace OCA\DAV\Connector\Sabre;

use OC\AppFramework\Http\Request;
use OC\FilesMetadata\Model\FilesMetadata;
use OCA\DAV\Connector\Sabre\Exception\InvalidPath;
use OCP\Constants;
use OCP\Files\ForbiddenException;
Expand Down Expand Up @@ -575,7 +576,9 @@ private function handleUpdatePropertiesMetadata(PropPatch $propPatch, Node $node
$propPatch->handle(
$mutation,
function (mixed $value) use ($accessRight, $knownMetadata, $node, $mutation, $filesMetadataManager): bool {
/** @var FilesMetadata $metadata */
$metadata = $filesMetadataManager->getMetadata((int)$node->getFileId(), true);
$metadata->setStorageId($node->getNode()->getStorage()->getCache()->getNumericStorageId());
$metadataKey = substr($mutation, strlen(self::FILE_METADATA_PREFIX));

// confirm metadata key is editable via PROPPATCH
Expand Down
5 changes: 5 additions & 0 deletions apps/files/lib/Listener/SyncLivePhotosListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

namespace OCA\Files\Listener;

use OC\FilesMetadata\Model\FilesMetadata;
use OCA\Files\Service\LivePhotosService;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventListener;
Expand Down Expand Up @@ -154,10 +155,14 @@ private function handleCopy(NodeCopiedEvent $event, Node $peerFile): void {
* We have everything to update metadata and keep the link between the 2 copies.
*/
$newPeerFile = $peerFile->copy($targetParent->getPath() . '/' . $peerTargetName);
/** @var FilesMetadata $targetMetadata */
$targetMetadata = $this->filesMetadataManager->getMetadata($targetFile->getId(), true);
$targetMetadata->setStorageId($targetFile->getStorage()->getCache()->getNumericStorageId());
$targetMetadata->setString('files-live-photo', (string)$newPeerFile->getId());
$this->filesMetadataManager->saveMetadata($targetMetadata);
/** @var FilesMetadata $peerMetadata */
$peerMetadata = $this->filesMetadataManager->getMetadata($newPeerFile->getId(), true);
$peerMetadata->setStorageId($newPeerFile->getStorage()->getCache()->getNumericStorageId());
$peerMetadata->setString('files-live-photo', (string)$targetFile->getId());
$this->filesMetadataManager->saveMetadata($peerMetadata);
}
Expand Down
3 changes: 3 additions & 0 deletions lib/private/FilesMetadata/FilesMetadataManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,14 @@ public function refreshMetadata(
int $process = self::PROCESS_LIVE,
string $namedEvent = '',
): IFilesMetadata {
$storageId = $node->getStorage()->getCache()->getNumericStorageId();
try {
/** @var FilesMetadata $metadata */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typing is not working?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things are typed to return this interface, but only the implementation has the setStorageId method because I didn't want to touch the public api

$metadata = $this->metadataRequestService->getMetadataFromFileId($node->getId());
} catch (FilesMetadataNotFoundException) {
$metadata = new FilesMetadata($node->getId());
}
$metadata->setStorageId($storageId);

// if $process is LIVE, we enforce LIVE
// if $process is NAMED, we go NAMED
Expand Down
17 changes: 17 additions & 0 deletions lib/private/FilesMetadata/Model/FilesMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class FilesMetadata implements IFilesMetadata {
private bool $updated = false;
private int $lastUpdate = 0;
private string $syncToken = '';
private ?int $storageId = null;

public function __construct(
private int $fileId = 0,
Expand All @@ -42,6 +43,22 @@ public function getFileId(): int {
return $this->fileId;
}

public function getStorageId(): ?int {
return $this->storageId;
}

/**
* Set which storage the file this metadata belongs to.
*
* This helps with sharded filecache setups to know where to store the metadata
*
* @param int $storageId
* @return void
*/
public function setStorageId(int $storageId): void {
icewind1991 marked this conversation as resolved.
Show resolved Hide resolved
$this->storageId = $storageId;
}

/**
* @inheritDoc
* @return int timestamp
Expand Down
18 changes: 18 additions & 0 deletions lib/private/FilesMetadata/Service/MetadataRequestService.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@ public function __construct(
) {
}

private function getStorageId(IFilesMetadata $filesMetadata): int {
if ($filesMetadata instanceof FilesMetadata) {
$storage = $filesMetadata->getStorageId();
if ($storage) {
return $storage;
}
}
// all code paths that lead to saving metadata *should* have the storage id set
// this fallback is there just in case
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to update applications like Photos

$query = $this->dbConnection->getQueryBuilder();
$query->select('storage')
->from('filecache')
->where($query->expr()->eq('fileid', $query->createNamedParameter($filesMetadata->getFileId(), IQueryBuilder::PARAM_INT)));
return $query->executeQuery()->fetchColumn();
Copy link
Contributor

@artonge artonge Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should cache it with $filesMetadata->setStorageId(), but I don't think that we have scenarios where we are saving the same metadata object multiple times.

}

/**
* store metadata into database
*
Expand All @@ -38,6 +54,7 @@ public function __construct(
public function store(IFilesMetadata $filesMetadata): void {
$qb = $this->dbConnection->getQueryBuilder();
$qb->insert(self::TABLE_METADATA)
->hintShardKey('storage', $this->getStorageId($filesMetadata))
->setValue('file_id', $qb->createNamedParameter($filesMetadata->getFileId(), IQueryBuilder::PARAM_INT))
->setValue('json', $qb->createNamedParameter(json_encode($filesMetadata->jsonSerialize())))
->setValue('sync_token', $qb->createNamedParameter($this->generateSyncToken()))
Expand Down Expand Up @@ -134,6 +151,7 @@ public function updateMetadata(IFilesMetadata $filesMetadata): int {
$expr = $qb->expr();

$qb->update(self::TABLE_METADATA)
->hintShardKey('files_metadata', $this->getStorageId($filesMetadata))
->set('json', $qb->createNamedParameter(json_encode($filesMetadata->jsonSerialize())))
->set('sync_token', $qb->createNamedParameter($this->generateSyncToken()))
->set('last_update', $qb->createFunction('NOW()'))
Expand Down
98 changes: 98 additions & 0 deletions tests/lib/FilesMetadata/FilesMetadataManagerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2024 Robin Appelman <[email protected]>
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace Test\FilesMetadata;

use OC\BackgroundJob\JobList;
use OC\Files\Storage\Temporary;
use OC\FilesMetadata\FilesMetadataManager;
use OC\FilesMetadata\Service\IndexRequestService;
use OC\FilesMetadata\Service\MetadataRequestService;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
use OCP\FilesMetadata\AMetadataEvent;
use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\Server;
use Psr\Log\LoggerInterface;
use Test\TestCase;
use Test\Traits\MountProviderTrait;
use Test\Traits\UserTrait;

/**
* @group DB
*/
class FilesMetadataManagerTest extends TestCase {
use UserTrait;
use MountProviderTrait;

private IEventDispatcher $eventDispatcher;
private JobList $jobList;
private IAppConfig $appConfig;
private LoggerInterface $logger;
private MetadataRequestService $metadataRequestService;
private IndexRequestService $indexRequestService;
private FilesMetadataManager $manager;
private IDBConnection $connection;
private Folder $userFolder;
private array $metadata = [];

protected function setUp(): void {
parent::setUp();

$this->jobList = $this->createMock(JobList::class);
$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
$this->eventDispatcher->method('dispatchTyped')->willReturnCallback(function (Event $event) {
if ($event instanceof AMetadataEvent) {
$name = $event->getNode()->getName();
if (isset($this->metadata[$name])) {
$meta = $event->getMetadata();
foreach ($this->metadata[$name] as $key => $value) {
$meta->setString($key, $value);
}
}
}
});
$this->appConfig = $this->createMock(IAppConfig::class);
$this->logger = $this->createMock(LoggerInterface::class);

$this->connection = Server::get(IDBConnection::class);
$this->metadataRequestService = new MetadataRequestService($this->connection, $this->logger);
$this->indexRequestService = new IndexRequestService($this->connection, $this->logger);
$this->manager = new FilesMetadataManager(
$this->eventDispatcher,
$this->jobList,
$this->appConfig,
$this->logger,
$this->metadataRequestService,
$this->indexRequestService,
);

$this->createUser('metatest', '');
$this->registerMount('metatest', new Temporary([]), '/metatest');

$rootFolder = Server::get(IRootFolder::class);
$this->userFolder = $rootFolder->getUserFolder('metatest');
}

public function testRefreshMetadata(): void {
$this->metadata['test.txt'] = [
'istest' => 'yes'
];
$file = $this->userFolder->newFile('test.txt', 'test');
$stored = $this->manager->refreshMetadata($file);
$this->assertEquals($file->getId(), $stored->getFileId());
$this->assertEquals('yes', $stored->getString('istest'));

$retrieved = $this->manager->getMetadata($file->getId());
$this->assertEquals($file->getId(), $retrieved->getFileId());
$this->assertEquals('yes', $retrieved->getString('istest'));
}
}
Loading