-
Notifications
You must be signed in to change notification settings - Fork 780
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
feat: enhance replay #2984
feat: enhance replay #2984
Conversation
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #2984 +/- ##
==========================================
- Coverage 52.55% 52.52% -0.03%
==========================================
Files 134 134
Lines 11901 11920 +19
==========================================
+ Hits 6254 6261 +7
- Misses 5155 5169 +14
+ Partials 492 490 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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
break | ||
case <-time.After(time.Second * 10): | ||
log.Error(fmt.Errorf("internal: background relist did not exit gracefully"), "possible goroutine leak") | ||
// do not close waitToCloseChan as the goroutine may eventually exit and call close on the channel |
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.
also, it will be GC'd anyway
Signed-off-by: alex <8968914+acpana@users.noreply.github.com>
waitToCloseChan := make(chan struct{}) | ||
|
||
// the 0th relist goroutine is stopped, by definition, so we close the channel. | ||
close(waitToCloseChan) |
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.
Can you share why we are closing it right after it's created? I'm not clear on the comment.
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.
iirc, without closing the channel the first time around, the select block below will fire off an error (after 10 seconds hanging on a channel that is not signaled yet).
does that make sense? if so, how could we make the comment better?
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.
@ritazh let me know if the updated comment address the why of not closing the relistStopChan
here. 🙏🏼
Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com> Signed-off-by: alex <8968914+acpana@users.noreply.github.com>
Signed-off-by: alex <8968914+acpana@users.noreply.github.com>
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
Thanks for addressing this!
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com> Signed-off-by: alex <8968914+acpana@users.noreply.github.com> Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>
fixes #2983