Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

sp-utils: tracing bounded channels #6609

Closed
wants to merge 2 commits into from
Closed

sp-utils: tracing bounded channels #6609

wants to merge 2 commits into from

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Jul 8, 2020

rel #6563

This adds bounded channel support for tracing channels, modeled similar to current unbounded tracing channel implementations.

@sorpaas sorpaas added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 8, 2020
@sorpaas sorpaas requested a review from gnunicorn July 8, 2020 21:59
Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Code-wise looks good to me, just trying to understand the rational (as this PR doesn't contain any case of it being used): with a channel that applies back-pressure, we can't really leak there. What is the intention of tracing the count of their messages?

@gnunicorn gnunicorn requested a review from mxinden July 9, 2020 09:28
@bkchr
Copy link
Member

bkchr commented Jul 9, 2020

Yes, I have the same question. What is the point of tracing a bounded channel?

@sorpaas
Copy link
Member Author

sorpaas commented Jul 10, 2020

I just thought it's good to have the consistency -- with all our unbounded channels being traced, maybe it's good to have bounded ones traced as well.

Maybe I'm entirely wrong, so not sure if we want to proceed with this PR. If not, let me update #6563 directly using bounded channels.

@gnunicorn
Copy link
Contributor

@sorpaas yeah, consistency is good, but in this case, the error is that we have unbounded channels. Bounded channels are fine and no source of leaks and if we had an entirely async system, we could just use them instead. Adding tracing was a patch to the situation being broken and making sure we don't leak.

IMHO we should focus on replacing the occurrences and get rid of the patch, rather than adding on it. If it is okay with you, I'd rather not merge this...

@sorpaas sorpaas closed this Jul 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants