Skip to content

query: add OnMaxTries #308

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

query: add OnMaxTries #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6d7733
query: add onmaxtries config option
kcalvinalvin Dec 6, 2024
30921a6
query: check if peer is disconnected before calling handleResponse
kcalvinalvin Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions query/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,18 @@ func (w *worker) Run(results chan<- *jobResult, quit <-chan struct{}) {
// response handler to check whether it was answering
// our request.
case resp := <-msgChan:
select {
// If the peer disconnects before giving us a valid
// answer, we'll also exit with an error.
case <-peer.OnDisconnect():
log.Debugf("Peer %v for worker disconnected, "+
"cancelling job %v", peer.Addr(),
job.Index())

jobErr = ErrPeerDisconnected
break Loop
Comment on lines +164 to +173
Copy link
Member

Choose a reason for hiding this comment

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

i dont think this adds anything. This select is already covered in below and so if peer.OnDisconnect() triggers before the message is received on the msgChan, we will already catch the case below.

I think what you instead want is for job.HandleResp's function to know if it can exit or not.

The idiomatic way of doing this would be to pass it a ctx context.Context which gets cancelled when the peer disconnects. An intermediate step can just be to pass it a quit chan struct{} which you can derive from the peers OnDisconnect method and then listen on that in the HandleResp call-back

Copy link
Author

Choose a reason for hiding this comment

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

So the reason for adding this is because I saw that there are cases where case resp := <- msgChan: gets triggered first before the below case <-peer.OnDisconnect(): gets triggered.

I think what you instead want is for job.HandleResp's function to know if it can exit or not.

Yeah that would also solve the problem I was seeing.

An intermediate step can just be to pass it a quit chan struct{} which you can derive from the peers OnDisconnect method

That would be ok as well except now we force every implementation of HandleResp to handle the case where quit chan struct{} is passed.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I'll also add that I don't really have a preference on how that's handled so I'll push changes so that HandleResp takes a quti chan struct{} as an argument.

default:
}
progress := job.HandleResp(
job.Req, resp, peer.Addr(),
)
Expand Down
8 changes: 8 additions & 0 deletions query/workmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ type Config struct {
// make this configurable to easily mock the worker used during tests.
NewWorker func(Peer) Worker

// OnMaxTries is function closure that's called on max retries on
// workers.
OnMaxTries func(string)
Copy link
Member

Choose a reason for hiding this comment

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

think we should instead use a stricter type here (net.Addr) and mention what is being passed to this call-back so that the caller doesnt have to go look at the code to figure out what is being passed to the call-back it is setting.

The commit message isnt very accurate imo. OnMaxTries config lets the caller choose what happens when a query goes over the maximum allowed tries isnt really true - more so: OnMaxTries gives the caller access to the peer's address once a maxmum number of retries have been attempted" or something like that

Copy link
Member

Choose a reason for hiding this comment

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

it's worth linking the PR that makes use of this new config option in the PR description so that reviewers can understand the context a bit more

Copy link
Author

Choose a reason for hiding this comment

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

think we should instead use a stricter type here (net.Addr) and mention what is being passed to this call-back so that the caller doesnt have to go look at the code to figure out what is being passed to the call-back it is setting.

The commit message isnt very accurate imo. OnMaxTries config lets the caller choose what happens when a query goes over the maximum allowed tries isnt really true - more so: OnMaxTries gives the caller access to the peer's address once a maxmum number of retries have been attempted" or something like that

Will address both points.


// Ranking is used to rank the connected peers when determining who to
// give work to.
Ranking PeerRanking
Expand Down Expand Up @@ -381,6 +385,10 @@ Loop:
log.Debugf("Canceled batch %v",
batchNum)

if w.cfg.OnMaxTries != nil {
w.cfg.OnMaxTries(result.peer.Addr())
}

continue Loop
}

Expand Down