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

feat(server): efficient full app sync #8755

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Conversation

fyfrey
Copy link
Contributor

@fyfrey fyfrey commented Apr 12, 2024

Adds two new endpoints (in a new controller + service) to sync assets for the mobile app.

Compared to the old approach, these DB queries do not use offset/skip and can make use of the available indices. This should prevent the server/DB from overloading during initial/full app assets sync.

Further, the fast/delta sync now includes all owned+partner assets (thereby reducing the number of network requests and full table scans!)

This PR does not include the mobile changes. These will merged separately after 1-2 releases/weeks have passed to avoid all apps from breaking because the server has not yet been updated.

Copy link

cloudflare-workers-and-pages bot commented Apr 12, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8074119
Status: ✅  Deploy successful!
Preview URL: https://88c8b8cd.immich.pages.dev
Branch Preview URL: https://feat-server-efficient-sync.immich.pages.dev

View logs

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This looks pretty good. We're trying to avoid putting everything into the asset controller though. What do you think about splitting these new endpoints out into a controller and service specifically for syncing instead? Maybe endpoints like /sync/assets-full and /sync/assets-delta or similar?

@fyfrey fyfrey marked this pull request as ready for review April 14, 2024 17:13
@fyfrey fyfrey requested a review from danieldietzler as a code owner April 14, 2024 17:13
Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM

server/src/services/sync.service.ts Outdated Show resolved Hide resolved
Co-authored-by: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com>
@alextran1502 alextran1502 requested a review from jrasm91 April 15, 2024 06:08
@alextran1502
Copy link
Contributor

Hello @fyfrey, thank you for another high-quality PR. Can you add some words to the description about how the full sync vs delta sync works? What are the different mechanisms used between the two?

@fyfrey
Copy link
Contributor Author

fyfrey commented Apr 15, 2024

Sure, @alextran1502! It's actually the same as the current mechanism (just more performant on the server side):

Full sync: The app gets all asset information, compares this list with the state in it's database and upserts/removes entries as needed. The list of all assets is fetched in chunks per user, but still made into one giant list per user on the client (so, for 100k+ assets this is still an issue).

Delta sync: The app gets all changed and deleted assets for the user and partners. The server calculates this using the updatedAt property and the audit table (for deletes). So the app retrieves a comparable small set of changes regardless of the library size. It updates its database accordingly.

The app always tries to do a delta sync and only uses the full sync as a fallback: For initial sync when the client DB is empty, if there are too many changes or if the last sync is too long ago.

@alextran1502 alextran1502 merged commit 103cb60 into main Apr 16, 2024
24 checks passed
@alextran1502 alextran1502 deleted the feat/server-efficient-sync branch April 16, 2024 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants