-
Notifications
You must be signed in to change notification settings - Fork 10
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the priority always starts at "max priority" and decreases from there:
So this is only useful if you're planning on running a private network and intend on setting the priority yourself.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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-L136There was a problem hiding this comment.
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.