-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
make eval cancelation async with Eval.Ack
#15294
Conversation
In #14621 we added an eval canelation reaper goroutine with a channel that allowed us to wake it up. But we forgot to actually send on this channel from `Eval.Ack` and are still committing the cancelations synchronously. Fix this by sending on the buffered channel to wake up the reaper instead.
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.
LGTM!
// Wake up the eval cancelation reaper | ||
e.srv.reapCancelableEvalsCh <- struct{}{} |
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.
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:
// 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).
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.
Your implementation isn't wrong though, just not always asynchronous.
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.
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.
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.
Take 2 (3?) is here: #15298
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
In #14621 we added an eval canelation reaper goroutine with a channel that allowed us to wake it up. But we forgot to actually send on this channel from
Eval.Ack
and are still committing the cancelations synchronously. Fix this by sending on the buffered channel to wake up the reaper instead.(ref https://github.com/hashicorp/nomad/pull/14621/files#r1025630828)