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(iroh-bytes): add initial query request #1413

Closed
wants to merge 1 commit into from
Closed

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Aug 26, 2023

Description

This adds a new request type that allows the requester to get availability information about a blob or collection without having to download the data.

Notes & open questions

Merge this before implementing so we keep the protocol stable?

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

not implemented yet, just the request and docs for it
@rklaehn
Copy link
Contributor Author

rklaehn commented Aug 26, 2023

First part of #1414

@Frando
Copy link
Member

Frando commented Aug 26, 2023

Question: Do we want to be able to batch these? I can imagine that oftenly you'd want to query for a lot of hashes. With this architecture, you'd send a query for each hash on a separate stream, so likely sequentially. When sending a batch request, the provider can potentially batch resolve the availability in parallel? Not sure, but thought it's worth to bring up the question.

iroh-bytes/src/protocol.rs Show resolved Hide resolved
iroh-bytes/src/protocol.rs Show resolved Hide resolved
iroh-bytes/src/protocol.rs Show resolved Hide resolved
}
}

/// Request just a single blob
Copy link
Contributor

Choose a reason for hiding this comment

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

this means "the first blob"?

@flub
Copy link
Contributor

flub commented Aug 28, 2023

Question: Do we want to be able to batch these? I can imagine that oftenly you'd want to query for a lot of hashes. With this architecture, you'd send a query for each hash on a separate stream, so likely sequentially. When sending a batch request, the provider can potentially batch resolve the availability in parallel? Not sure, but thought it's worth to bring up the question.

Batching on one stream vs creating many streams is something I often wonder and still haven't benchmarked. In theory it's nicer to use many streams, the sender can create an entire bunch of requests in a tight loop and quinn should start packing these streams together into packets so you don't really suffer from the many streams. That's the theory at least. As I said I don't know how well it works in practice.

Also the quinn folks once mentioned they were wondering about implementing something like tcp_cork on a stream and/or connection to even further improve this: the ability to say to quinn "only start sending things once you have full packets or i uncork". That would improve that kind of behaviour even more.

@flub
Copy link
Contributor

flub commented Aug 28, 2023

Merge this before implementing so we keep the protocol stable?

Not sure I follow? You want to get this into today's release?

Afaik this is a new request type so should be ok to add later is what we were saying last week i think?

@b5 b5 added this to the v0.6.0-alpha2 milestone Aug 29, 2023
@rklaehn
Copy link
Contributor Author

rklaehn commented Aug 29, 2023

Merge this before implementing so we keep the protocol stable?

Not sure I follow? You want to get this into today's release?

Afaik this is a new request type so should be ok to add later is what we were saying last week i think?

I had just briefly considered having this request in there. But it's fine to do it later.

@rklaehn
Copy link
Contributor Author

rklaehn commented Aug 29, 2023

Question: Do we want to be able to batch these? I can imagine that oftenly you'd want to query for a lot of hashes. With this architecture, you'd send a query for each hash on a separate stream, so likely sequentially. When sending a batch request, the provider can potentially batch resolve the availability in parallel? Not sure, but thought it's worth to bring up the question.

For the entire protocol, I have assumed that batching is done by either creating a collection containing related items, or opening multiple quinn streams. The latter should be cheap for an existing connection.

Batching would be a straightforward extension of the protocol, reusing things like RangeSpecSeq etc., but it definitely requires more thought. So I would prefer if we try how far we can get without batching with cheap quinn substreams - just like in quic-rpc a RPC call creates it's own substream.

@rklaehn
Copy link
Contributor Author

rklaehn commented Aug 29, 2023

Batching on one stream vs creating many streams is something I often wonder and still haven't benchmarked. In theory it's nicer to use many streams, the sender can create an entire bunch of requests in a tight loop and quinn should start packing these streams together into packets so you don't really suffer from the many streams. That's the theory at least. As I said I don't know how well it works in practice.

Anecdotal evidence, but quic-rpc is using a stream per RPC call, and nevertheless you can get several 100000 RPC calls per second giving a fast connection.

If you look at RPC (1 stream per request) vs bidi (1 stream for all requests), RPC par isn't that much worse than bidi seq. This is what I base my gut feeling on that streams are cheap.

RPC seq is waiting for the response before sending the next request, so it is clear why it is slower.

> cargo test --release quinn_channel_bench -- --nocapture
running 1 test
RPC seq 22_567 rps                                                              
RPC par 193_088 rps                                                             
bidi seq 252_977 rps                                                            
test quinn_channel_bench ... ok

@b5 b5 modified the milestones: v0.6.0-alpha2, v0.6.0 Aug 31, 2023
@divagant-martian divagant-martian mentioned this pull request Sep 5, 2023
3 tasks
github-merge-queue bot pushed a commit that referenced this pull request Sep 13, 2023
## Description

Adds the `Downloader` as specified in #1334 plus some backchannel convos
Features include:
- Support collections
- Add delays to downloads
- Add download retries with an incremental backoff 
- Keeping peers for a bit longer than necessary in hopes they will be
useful again
- Having the concept of intents and deduplicating downloads efforts
- Cancelling download intents
- Limiting how many concurrent requests are done in total
- Limiting how many concurrent requests are done per peer
- Limiting the number of open connections in total
- Basic error management in the form of deciding whether a peer should
be dropped, the request should be dropped, or if the request should be
retried

## Notes & open questions
### TODOs
- A remaining TODO in the code is whether something special should be
done when dropping quic connections
- Should downloads have a timeout?
- ~<sup>I know I've said this a hundred times with a hundred different
things but would love to test this as well under stress scenarios and a
large number of peers. don't hate me</sup>~
In reality after abstracting away all the IO most scenarios can be
simulated easily. What would remain for a _much_ later time when the
need and opportunity for real case testing scenario arises is to tune
the concurrency parameters

### Future work
#### Downloading Ranges
There was the requirement of downloading a Hash, a range of a Hash, a
collection and (not mentioned but potentially implied) ranges of
collections. There is no support for ranges right now because of the
great duplication of the `get` code in order to take advantage of proper
errors added in #1362. In principle, adding ranges should be really
easy. This is because it's an extension of the `DownloadKind` and would
simply need calculating the missing ranges not based on the difference
between what we have and the whole range, but the given range. I would
prefer to find a way to deduplicate the get code before doing this
extension.
Also, as far as I can tell, there is no need for this yet.
#### Prioritizing candidates per role: `Provider` and `Candidate`
A nice extension, as discussed at some point, is to differentiate
candidates we know have the data, from those that _might_ have the data.
This has added benefit that when a peer is available to perform another
download under the concurrency limits, a hash we know they have could be
downloaded right away instead of waiting for the delay. At this point
making this doesn't make sense because we will likely attempt a download
before the peer has retrieved the data themselves. To implement this, we
would need to add the notifications of fully downloaded hashes as
available into gossip first.
#### Leveraging the info from gossip
When declaring that a hash `X` should be downloaded, it's also an option
to query gossip for peers that are subscribed to the topic to which `X`
belongs to use them as candidates. This could be done connecting the
`ProviderMap` to `gossip`. For now I don't see the need to do this.

### Open questions about Future work
- In line with the described work from above, the registry only allows
querying for peer candidates to a hash since that's as good as it gets
in terms of what we know from a remote right now. It's not clear to me
if we would want to change this to have better availability information
with #1413 in progress.
- More future work: downloading a large data set/blob from multiple
peers would most likely require us to do a three step process:
  1. understanding the data layout/size.
  2. splitting the download.
  3. actually performing the separate downloads.
Generally curious how this will end. My question here is whether we
should do this for every download, or just on data that we expect to be
big. Is there any way to obtain such hint without relying on a query
every single time?


## Change checklist

- [x] Self-review.
- [x] Documentation updates if relevant.
- [x] Tests if relevant.
@ramfox ramfox modified the milestones: v0.6.0, v0.7.0 Sep 22, 2023
@dignifiedquire
Copy link
Contributor

what's the state of this?

@b5 b5 modified the milestones: v0.7.0, v0.8.0 Sep 29, 2023
@dignifiedquire dignifiedquire removed this from the v0.9.0 milestone Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants