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

Adds customizable prioritization logic for peertracker and peertaskqueue #17

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

synzhu
Copy link
Contributor

@synzhu synzhu commented Oct 11, 2021

This is one step towards allowing configurable prioritization of requests for bitswap: ipfs/boxo#82

Adds new configuration options for customizing peer prioritization and queue task prioritization.

@@ -108,6 +129,22 @@ func PeerCompare(a, b pq.Elem) bool {
return pa.activeWork < pb.activeWork
}

// TaskPriorityPeerComparator prioritizes peers based on their highest priority task.
func TaskPriorityPeerComparator(comparator peertask.QueueTaskComparator) PeerComparator {
Copy link
Member

Choose a reason for hiding this comment

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

I assume you're using this on a trusted network?

Copy link
Contributor Author

@synzhu synzhu Oct 12, 2021

Choose a reason for hiding this comment

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

Not exactly, but is that actually a problem here? Are you worried that peers may lie about their priority?

I provide the comparator function myself, which does not necessarily need to compare two tasks based on what the requesters say their priority is. It could be based on something else, such as the data being requested.

Copy link
Member

Choose a reason for hiding this comment

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

Not exactly, but is that actually a problem here? Are you worried that peers may lie about their priority?

Well, the priority always starts at "max priority" and decreases from there:

  1. Yes, a peer could just send everything with "max priority".
  2. Longer-lived peers will always have "less" priority.

So this is only useful if you're planning on running a private network and intend on setting the priority yourself.


I provide the comparator function myself, which does not necessarily need to compare two tasks based on what the requesters say their priority is. It could be based on something else, such as the data being requested.

Got it. In that case, any objections to removing TaskPriorityPeerComparitor? I'm a bit concerned someone will misuse it and wonder why everything is broken.

Copy link
Contributor Author

@synzhu synzhu Oct 12, 2021

Choose a reason for hiding this comment

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

So this is only useful if you're planning on running a private network and intend on setting the priority yourself.

Indeed, I was only intending to use this when supplying a QueueTaskComparator which does comparison based on something other than the priority sent by a peer. Maybe I should go farther and explicitly document that this is the intended usecase? Perhaps something like:

// TaskPriorityPeerComparator prioritizes peers based on their highest priority task, where priorities
// are determined based on the given QueueTask comparison function, and NOT the priority specified
// by the client.

Or I can rename it to something else if you think the naming is confusing. But yeah, essentially how this is supposed to work is, I provide a QueueTask comparison function, and then Peer comparison logic will be: "the peer with higher priority is the one whose highest priority task has higher priority according to the provided QueueTask comparison function". Hope this makes sense?

If I remove it, I will not be able to achieve this usecase. It's not possible atm for me to define this outside of the package, because taskQueue is private: https://github.com/smnzhu/go-peertaskqueue/blob/9432115230b5ce765a9f08c46d6a1dc67b65798f/peertracker/peertracker.go#L135-L136

Copy link
Member

Choose a reason for hiding this comment

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

No, no. I just somehow completely failed to actually see the code I was reviewing. I need to pay more attention, this code is fine.

@Stebalien Stebalien merged commit 03bcda6 into ipfs:master Oct 12, 2021
@Stebalien
Copy link
Member

Thanks!

@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

2 participants