Skip to content
This repository has been archived by the owner on Aug 23, 2019. It is now read-only.

feat: support a priority queue for dials #325

Merged
merged 8 commits into from
Apr 10, 2019
Merged

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Apr 9, 2019

This adds basic support for priorities. Currently, all priority 0 (highest) items will be run through the main dial queue and all priorities >= 1 will go through the cold call queue. This enables the proposed auto dial feature in libp2p, libp2p/js-libp2p#349, to create low priority dials on peer discovery.

Since some applications may dial to peers without specifying a protocol, such as dialing to a content provider discovered through the DHT, only detecting the presence of a protocol is not sufficient to determine priority. switch.dialer.connect creates a direct path for applications to use the cold call queue.

required by libp2p/js-libp2p#349

@ghost ghost assigned jacobheun Apr 9, 2019
@ghost ghost added the in progress label Apr 9, 2019
@jacobheun jacobheun force-pushed the feat/priority-queue branch from 87aa0c1 to b00d6cd Compare April 9, 2019 17:26
@jacobheun jacobheun changed the title [WIP] feat: support a priority queue for dials feat: support a priority queue for dials Apr 10, 2019
@jacobheun jacobheun marked this pull request as ready for review April 10, 2019 08:55
Copy link

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

One small suggestion, otherwise LGTM 👍

README.md Outdated

- `peer`: can be an instance of [PeerInfo][], [PeerId][] or [multiaddr][]
- `options`: Optional
- `options.priority`: Number of the priority of the dial, defaults to 1.
Copy link

Choose a reason for hiding this comment

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

Should we start with higher numbered priorities to leave some wiggle room in case there are other use cases in future? eg HIGH=10, LOW=20, and then in future maybe there's a MEDIUM=15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of anything now, but that doesn't mean there wont be :). I've added those as constants to the constants file, and am now referencing them for the options.

Copy link
Member

Choose a reason for hiding this comment

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

oh ups, I did not see this review discussion and left a comment in this regard too

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Looks good to me! Only left 2 minor comments

README.md Outdated

- `peer`: can be an instance of [PeerInfo][], [PeerId][] or [multiaddr][]
- `options`: Optional
- `options.priority`: Number of the priority of the dial, defaults to 1.
Copy link
Member

Choose a reason for hiding this comment

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

What is the acceptable range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since items currently go into 1 of 2 queues that execute by order of adds, I updated the docs to try to be more clear about what's going on. I think if we expand on that in the future or actually add priority to each of the queues we can lock down what's acceptable.

options = { useFSM: false, priority: 1, ...options }
_dial({ peerInfo, protocol: null, options, callback })
}

/**
* Adds the dial request to the queue for the given `peerInfo`
Copy link
Member

Choose a reason for hiding this comment

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

Add docs for using higher priority here

@jacobheun jacobheun merged commit 0357bf2 into master Apr 10, 2019
@jacobheun jacobheun deleted the feat/priority-queue branch April 10, 2019 17:22
@ghost ghost removed the in progress label Apr 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants