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

chore: Send download intent to peers #944

Merged
merged 2 commits into from
Oct 30, 2024
Merged

chore: Send download intent to peers #944

merged 2 commits into from
Oct 30, 2024

Conversation

gmaclennan
Copy link
Member

Fixes #686

@gmaclennan gmaclennan self-assigned this Oct 29, 2024
@gmaclennan gmaclennan marked this pull request as ready for review October 29, 2024 12:45
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just a few small things. I definitely prefer the name "download intent".

Are there any tests we could add for this?


/**
*
* @param {object} opts
* @param {import('../core-manager/index.js').CoreManager} opts.coreManager
* @param {CoreOwnership} opts.coreOwnership
* @param {import('../roles.js').Roles} opts.roles
* @param {import('../types.js').BlobFilter | null} opts.blobDownloadFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we always set this to null. Can we either (1) set the option (2) remove the option?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is waiting for #940 to land so we can use the NON_ARCHIVE_DEVICE_FILTER constant for initializing the class. Left it out to avoid creating a dependency on that PR for landing this.

src/sync/sync-api.js Outdated Show resolved Hide resolved
@gmaclennan
Copy link
Member Author

We could add temporary tests, which dive into the implementation details and list to events in the core manager, or we can wait until we update sync wants based on the download intents which will enable us to write end-to-end tests. I'm tempted to just wait for e2e tests.

@gmaclennan gmaclennan merged commit 97be4b9 into main Oct 30, 2024
8 checks passed
@gmaclennan gmaclennan deleted the chore/want-hints branch October 30, 2024 03:00
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.

Send "want hints" to peers based on sync download setting
2 participants