-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[cmd/opampsupervisor] Fix restart delay when agent process exits unexpectedly #30238
Conversation
Thank you for taking care of this. Could you add a changelog entry? |
I think the failed test is relevant. I will fix it. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
@srikanthccv have you had a chance to look at the failing test?
Yes @codeboten, There were timers in other places as well. I will test out the changes and push the fix soon. |
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.
Overall this looks good, most of my comments are just to clean things up for the future.
Could you add a changelog entry?
I noticed I already added the CHANGELOG entry. PTAL. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Not stale, I didn't get a chance to look into the review comments yet. I will get back to this soon. |
c.doneCh = make(chan struct{}, 1) | ||
c.exitCh = make(chan struct{}, 1) |
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 think we should just create these once on startup then drain them above. That way we can let the caller figure out multicasting.
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.
Sorry for tripping, I was reactively making changes to comments. Let me go through the changeset again and address the comment.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
Reset should called only on stopped or expired timers with drained channels. If the timer already expired (and the channel was not cleared) it reads from the timer's channel to clear it.
Link to tracking Issue:
Fixes #27891
Testing:
Manually verified while working on something else