Skip to content

Conversation

@b5
Copy link
Member

@b5 b5 commented Oct 2, 2025

Description

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@b5 b5 changed the title WIP add example for creating & fetching a collection Oct 2, 2025
@github-actions
Copy link

github-actions bot commented Oct 2, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh-blobs/pr/176/docs/iroh_blobs/

Last updated: 2025-10-07T07:04:05Z

@b5 b5 force-pushed the b5/collection_fetch_example branch 2 times, most recently from cc64450 to 703fd2a Compare October 2, 2025 20:04
@n0bot n0bot bot added this to iroh Oct 2, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Oct 2, 2025
@b5 b5 force-pushed the b5/collection_fetch_example branch from 703fd2a to 8a00f47 Compare October 2, 2025 20:28

impl Node {
async fn new() -> Result<Self> {
let endpoint = Endpoint::builder().bind().await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm skipping .discovery_n0() here and using add_node_addr directly on the endpoint in the Node::get_collection method below. Largely to take a highligher to the add_node_addr discussion we've been having.

@rklaehn: the store has a connection pool, but I can't seem to get at it? Seems the connection pool relies on me being able to plumb addresses into the endpoint, is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The store doesn't have anything, not even an endpoint. The downloader has a connection pool.

/// retrive an entire collection from a given hash and provider
async fn get_collection(&self, hash: Hash, provider: NodeAddr) -> Result<()> {
self.router.endpoint().add_node_addr(provider.clone())?;
let req = HashAndFormat::hash_seq(hash);
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 magical incantation to feed to download as a req param was super tough to figure out. Maybe we just handle this with examples.

async fn get_collection(&self, hash: Hash, provider: NodeAddr) -> Result<()> {
self.router.endpoint().add_node_addr(provider.clone())?;
let req = HashAndFormat::hash_seq(hash);
let addrs = Shuffled::new(vec![provider.node_id]);
Copy link
Member Author

Choose a reason for hiding this comment

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

I was surprised while working with store.downloader().download()that theContentDiscoverytrait worked inNodeIdand notNodeAddr`, is there a reason why?

Second: would it be possible to set it up so I can pass a Vec<NodeId> / Vec<NodeAddr>, and call .into() on it to get a Shuffled content discovery? That might help remove the concept of Content Discovery entirely from what we'd pass to download.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was surprised while working with store.downloader().download()that theContentDiscoverytrait worked inNodeIdand notNodeAddr`, is there a reason why?

I think it is nicer to let the downloader deal only with node ids and let the node discovery figure out how to dial the nodes.

I guess you could use NodeAddr in the public API, and then just call endpoint.add_node_addr before handing it over to the lower levels. But I am very much convinced that the lower levels should not do NodeAddrs.

Any large scale content discovery mechanism will only ever work with node ids, since node ids are longer time stable than addrs.

Second: would it be possible to set it up so I can pass a Vec / Vec, and call .into() on it to get a Shuffled content discovery? That might help remove the concept of Content Discovery entirely from what we'd pass to download.

I don't quite get this. You don't always want shuffled. And since ContentDiscovery itself is an infinite stream of node ids you can't make this a combinator - no way to shuffle an infinite stream.

I guess we could maybe make a builder for the common case where you have a finite set of node ids, where you would have shuffled as a combinator.

Comment on lines +97 to +98
self.store
.downloader(self.router.endpoint())
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm super confused about why I have to re-pass the endpoint to the store here to create a downloader. I guess I'm supposed to make a downloader & store it on the Node struct I've defined here?

It generally feels like a higher-level API is missing that just falls back to a DefaultDownloader

Copy link
Collaborator

Choose a reason for hiding this comment

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

The store can be used without an endpoint. There are many very valid use cases for this, e.g. you have a local store and want to do something without ever listening on the network.

I guess you could do a thing which combines a store and an endpoint, but then that is what the downloader is. Maybe there needs to be a thing that combines a store, endpoint and downloader.

I want to keep the downloader as a separate thing though since it has some internal state and there might be situations where you want to create/destroy them or even have multiple independent of the store.

Copy link
Collaborator

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

All fine except for that one bug with the collection tag.

@rklaehn rklaehn marked this pull request as ready for review October 7, 2025 06:50
@rklaehn rklaehn changed the title add example for creating & fetching a collection docs: add example for creating & fetching a collection Oct 7, 2025
@rklaehn rklaehn merged commit 9e4478e into main Oct 7, 2025
43 of 44 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants