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: introduce initial snapshot chunking #2119

Merged
merged 16 commits into from
Dec 10, 2024

Conversation

icehaunter
Copy link
Contributor

This PR changes the way we write snapshots to disk, the storage API to request them, and some logic around offsets.

  1. The Storage.get_snapshot/1 method is gone. It's now unified under Storage.get_log_stream/3, with some caveats.
  2. The offsets within the snapshot rows are not updated, so now latest item on the sent log may have mismatching offset field to the electric-offset header of the same response. All our clients respect the header, and that's the only correct behaviour going forward. There is a decision to remove offset from our items entirely, but that's a separate PR.
  3. While we're making the snapshot, we cannot know how many chunks are going to be there until we make them. Thus we allow clients to request any chunks up to LogOffset.new(0, :infinity), and consider all offsets at txn 0 to be virtual pointers.
  4. The get_log_stream/3 for the snapshot section ignores the contract a little bit - you cannot request the stream to be up to an arbitrary point, it's always up to the chunk boundary, even when specified otherwise. Since we've moved to "pure" chunking behaviour, it should be enshrined in the Storage interface so that reading functions can act optimally and honestly. I'm hoping to address that in a separate PR
  5. If the instance is started with the old snapshot already created, we update the naming and metadata to avoid recreating shapes if we can avoid it.

@icehaunter icehaunter force-pushed the ilia/feat/chunked-snapshot-serving branch from 479c877 to f2b9b34 Compare December 8, 2024 17:07
@icehaunter icehaunter force-pushed the ilia/feat/chunked-snapshot-serving branch from f2b9b34 to e302409 Compare December 8, 2024 17:13
@icehaunter icehaunter force-pushed the ilia/feat/chunked-snapshot-serving branch from e302409 to 1ebf667 Compare December 8, 2024 19:24
Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit c531cfc
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/67568b3107c1db00082d5e10
😎 Deploy Preview https://deploy-preview-2119--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@icehaunter icehaunter force-pushed the ilia/feat/chunked-snapshot-serving branch from c531cfc to 4d12065 Compare December 9, 2024 06:17
@icehaunter icehaunter force-pushed the ilia/feat/chunked-snapshot-serving branch from 4d12065 to e4a1074 Compare December 9, 2024 06:22
@icehaunter
Copy link
Contributor Author

icehaunter commented Dec 9, 2024

@msfstef can you please take another look at .skip-ed tests in the TS client that deal with headers and confirm that this me disabling them is correct? The general idea is "well we're serving only full chunks when serving snapshot, and up to file boundary, so switching to 'live' log is a separate HTTP request, hence e-tags should no longer mutate because response no longer mutates"

Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Very nice work! Love the ripping out of snapshot logic in the serve plug!

Left some comments and questions - as for the TS tests checking etags, we can modify them to be looking at the first "incomplete" actual log chunk rather than looking at -1 (which used to collate the transaction log)

packages/sync-service/mix.exs Outdated Show resolved Hide resolved
packages/sync-service/mix.exs Outdated Show resolved Hide resolved
packages/typescript-client/test/integration.test.ts Outdated Show resolved Hide resolved
packages/typescript-client/test/integration.test.ts Outdated Show resolved Hide resolved
@icehaunter icehaunter force-pushed the ilia/feat/chunked-snapshot-serving branch from 101a968 to 1a28bea Compare December 10, 2024 09:02
@icehaunter icehaunter merged commit 6ca47df into main Dec 10, 2024
31 checks passed
@icehaunter icehaunter deleted the ilia/feat/chunked-snapshot-serving branch December 10, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants