-
Notifications
You must be signed in to change notification settings - Fork 3
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
chore: blobStore.setDownloadFilter()
& createEntriesReadableStream()
#940
Conversation
blobStore.setDownloadFilter()
& createEntriesReadableStream()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean implementation. Left a bunch of comments but LGTM overall.
const coreManager = createCoreManager(...args) | ||
const blobStore = new BlobStore({ coreManager }) | ||
return { blobStore, coreManager } | ||
test('blobStore.createEntriesReadStream({ live: false })', async (t) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said earlier, do we ever (plan to) use live: false
? If not, could we remove this test?
8287f62
to
6496672
Compare
Before this change, we turned filters into a list of path prefixes and then checked entry paths against those prefixes. Rough pseudocode: ```javascript function doesEntryMatchFilter({ path }, filter) { const pathPrefixes = pathPrefixesFromFilter(filter) return pathPrefixes.some((prefix) => path.startsWith(prefix)) } ``` For performance and simplicity, I think it's cleaner if we "look up" entry paths in the existing filter object. Rough pseudocode: ```javascript function doesEntryMatchFilter({ path }, filter) { const { type, variant } = parsePath(path) return filter[type]?.includes(variant) } ``` I think this has two advantages: - It's less code. We don't need to worry about de-duping paths (e.g., `/photos/original` versus `/photos/`). We don't need to worry about sketchy paths (e.g., `/photos/garbage/../original`). - It's faster (at least, in theory). Rather than having to iterate over every path prefix, we only need to iterate over the variants of each path. (This could be further optimized with a `Set`.)
Added end-to-end tests in d374b20. This will currently break if deployed. If you set yourself as a non-archive device, sync will be partially broken. I'm going to merge this into the |
…)` (#940) - Removes `blobStore.download()` and the `LiveDownload` class. - Adds `blobStore.createEntriesReadableStream()` which is a stream of all entries in the blob store, filtered by blob type and variant(s). - Changes behaviour of `BlobStore` to auto-download blobs according to a `downloadFilter`. - Changes the `BlobStore` filter according to `isArchiveDevice` setting. - Changes the behaviour of `CoreManager` to not auto-download blob cores (this responsibility is now with `BlobStore`). Closes [#681]. [#681]: #681 Co-authored-by: Evan Hahn <[email protected]>
This is a squashed commit of: - #940 - #957 - #956 Co-authored-by: Gregor MacLennan <[email protected]>
Fixes #681
blobStore.download()
and theLiveDownload
class.blobStore.createEntriesReadableStream()
which is a stream of all entries in the blob store, filtered by blob type and variant(s).BlobStore
to auto-download blobs according to adownloadFilter
.BlobStore
filter according toisArchiveDevice
setting.CoreManager
to not auto-download blob cores (this responsibility is now withBlobStore
).TODO: