refactor(server): group async calls in metadata extraction #16450
Conversation
mertalev
force-pushed
the
refactor/server-metadata-extraction-group-async-calls
branch
2 times, most recently
from
3f9c211 to
a88e6bd
Compare
use debugFn no need to change mock
mertalev
force-pushed
the
refactor/server-metadata-extraction-group-async-calls
branch
from
a88e6bd to
ecdb914
Compare
|
I tested #14277 and refined the different parts of it in each split PR. I haven't tested the individual PRs extensively, but did confirm they work locally with one or two assets. |
There was a problem hiding this comment.
As far as I can tell this looks good. However I do think there are a lot of changes here, so it'd be good to get some testing done with a range of assets before we merge this and check everything is still handled correctly. Specifically around geo and also tag handling for a number tag or a tag with embedded pipes and slashes
| @@ -491,7 +492,7 @@ describe(MetadataService.name, () => { | |||
| }); | |||
| expect(mocks.user.updateUsage).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.ownerId, 512); | |||
| expect(mocks.storage.createFile).toHaveBeenCalledWith(assetStub.livePhotoMotionAsset.originalPath, video); | |||
| expect(mocks.asset.update).toHaveBeenNthCalledWith(1, { | |||
There was a problem hiding this comment.
What is the reason for changing all of these tests to remove the check that they were only called once?
There was a problem hiding this comment.
They don't check that they were called once, but that the e.g. 1st call looked like this. Since the calls are grouped, there's no clear order anymore. I can make it check that the total call count matches though.
| private async mergeExifTags(asset: AssetEntity): Promise<ImmichTags> { | ||
| const [mediaTags, sidecarTags, videoTags] = await Promise.all([ | ||
| this.metadataRepository.readTags(asset.originalPath), | ||
| asset.sidecarPath ? this.metadataRepository.readTags(asset.sidecarPath) : null, |
There was a problem hiding this comment.
Why not have this return {} still, now we need extra handling lower down to deal with something we realistically don't care about (the null case).
There was a problem hiding this comment.
Good call, making them null is a holdover from an earlier version of the PR.
Description
This PR refactors metadata extraction to increase concurrent asynchronous operations as well as making smaller code improvements, improving overall performance. It is based on #16390, which should be merged first.
Split off from #14277