Skip to content

Conversation

@leoyvens
Copy link
Collaborator

@leoyvens leoyvens commented Mar 30, 2022

As part of #3072, this adds the components to poll IPFS for the availability of a file. This does not track unavailability, once a file is found it is not polled again.

The load management approach is to have a configurable capacity for the IPFS service, which is shared among all subgraphs, and then a per-subgraph queue of pending requests. This should effectively distribute the service capacity fairly among subgraphs.

The implementation has two pieces, the monitoring algorithm which is generic over any tower::Service and an implementation of Service for IPFS.

Copy link
Contributor

@evaporei evaporei left a comment

Choose a reason for hiding this comment

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

The logic seems correct, I've added a bunch of comments for further discussion 🙂

}

#[tokio::test]
async fn polling_monitor_unordered() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I've followed how this tests the "unorderedness" of the PollingMonitor since the test does indeed send/receive in a specific order, can you explain it a bit further please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It sends in the order req-0, req-1, meaning req-1 is called first on the service. But the response for req-0 arrives first and is received without waiting for req-1. This tests that we're using .call_all(stream).unordered().

@leoyvens leoyvens force-pushed the leo/file-monitor branch 2 times, most recently from d4c0464 to f99155c Compare March 31, 2022 15:49
@leoyvens
Copy link
Collaborator Author

leoyvens commented Apr 1, 2022

There's no rush to merge this, so I'll leave it as PR for a while to see if tower 0.5 is released in the meantime.

Note to self: Whitelist the multihashes to cryptographically safe ones before merging.

Copy link
Member

@Theodus Theodus left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +58 to +71

{
let queue = queue.cheap_clone();
graph::spawn(async move {
let mut responses = service.call_all(queue_to_stream).unordered().boxed();
while let Some(response) = responses.next().await {
match response {
Copy link
Member

@Theodus Theodus Jul 14, 2022

Choose a reason for hiding this comment

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

For each PollingMonitor<S>, I think it would make sense to have metrics for:

  • queue depth (possibly overkill depending on how we use this)
  • timeout rate
  • error rate

@leoyvens leoyvens mentioned this pull request Jul 25, 2022
6 tasks
@leoyvens leoyvens merged commit 0cf060e into master Jul 25, 2022
@leoyvens leoyvens deleted the leo/file-monitor branch July 25, 2022 23: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.

4 participants