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.

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

Jump to bottom

make eval cancelation async with Eval.Ack #15294

Merged
merged 1 commit into from
Nov 17, 2022
Merged

make eval cancelation async with Eval.Ack #15294

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 3 additions & 4 deletions nomad/eval_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,9 @@ func (e *Eval) Ack(args *structs.EvalAckRequest,
return err
}

// It's not necessary to cancel evals before Ack returns, but it's done here
// to commit canceled evals as close to the Ack'd eval being committed as
// possible.
return cancelCancelableEvals(e.srv)
// Wake up the eval cancelation reaper
e.srv.reapCancelableEvalsCh <- struct{}{}
Comment on lines +237 to +238
Copy link
Member

Choose a reason for hiding this comment

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

This could still block if a raft apply is already in progress. I think to asynchronously trigger another loop you want to use a buffered chan:

Suggested change
// Wake up the eval cancelation reaper
e.srv.reapCancelableEvalsCh <- struct{}{}
// Wake up the eval cancelation reaper
select {
case e.srv.reapCancelableEvalsCh <- struct{}{}:
default:
}

and make the chan buffered of size 1: make(struct{}, 1)

This ensures Eval.Ack never blocks but that the cancellation loop always runs at least once after an Eval.Ack (either due to the Ack causing a send, or because a prior Ack's sent is already buffered and waiting to be recv'd).

Copy link
Member

Choose a reason for hiding this comment

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

Your implementation isn't wrong though, just not always asynchronous.

Copy link
Member Author

Choose a reason for hiding this comment

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

The channel is already buffered, with the thinking that we wanted to wait until the buffer was empty before continuing. But you're right we could avoid blocking entirely in that case b/c we already know the buffer is full. I merged right before your comment unfortunately, but I can fix that up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Take 2 (3?) is here: #15298

return nil
}

// Nack is used to negative acknowledge completion of a dequeued evaluation.
Expand Down