Repository navigation
Proposal: Require Streams WG to sign off on streams PRs #6178
Description
This came up in our latest Streams WG meeting. Recently there's been a couple of regressions after merging streams related PRs. Some of those were merged (or about to be merged) without a member of @nodejs/streams reviewing and signing off on it. The number of members in the WG is still relatively small (we are actively looking for more people to join) and the stream submodule has a complicated state machine and very few tests (we are working on improving this as well) so changes require very throughrough reviews.
Personally I've recently spent a lot of energy rushing to review PRs / going through commit history to avoid/catch unintended stream regressions being merged.
We would therefore like to propose to have it be a requirement that a @nodejs/streams member signs off on a stream related PR before it is merged until we can improve the current situation.
Activity
bnoordhuis commentedon Apr 13, 2016
A 'yes' is going to hinge on mean and median review time. Getting sign-off from WG people shouldn't become a limiting factor in pull request velocity.
mscdex commentedon Apr 13, 2016
I agree with Ben.
calvinmetcalf commentedon Apr 13, 2016
@mscdex you've been doing great adding labels to the issues, if you pinged @nodejs/streams when you added a streams label, that would make it much easier for members of streams to review them as early as possible
@bnoordhuis that hopefully shouldn't happen but if it does something like "ping @nodejs/streams last call for input or in 2 business days i'm merging this)
mcollina commentedon Apr 13, 2016
I see three problems here:
So, to address 1, we are scheduling meeting every month. We are looking for more people to join the wg (hey folks, we need help!), and we spent half an hour trying to figure out a plan to avoid damaging things in userland.
We need automatic pinging on subsystem, this is no task a human can do. See nodejs/github-bot#1.
mscdex commentedon Apr 13, 2016
I definitely think having a bot automatically ping would be better than relying on individual(s).
Fishrock123 commentedon Apr 13, 2016
For bot suggestions, please see the likes of nodejs/github-bot#1
Perhaps we can smoke-test mode streams users using citgm before landing streams commits? cc @thealphanerd
jasnell commentedon Apr 13, 2016
Should we also be putting additional effort into improving / expanding the test coverage for streams to help catch these in CI?
calvinmetcalf commentedon Apr 13, 2016
@jasnell yes that was what was exactly what we wanted to do, to hopefully avoid the need for this in the future.
calvinmetcalf commentedon Apr 13, 2016
ok not sure where to announce this, but since I have you all here, for the purpose of smoke testing, setting READABLE_STREAM=disable as an environmental variable will now cause readable stream to just return the system version of node which should allow us to smoke test libraries that rely on readable stream
jbergstroem commentedon Apr 14, 2016
I don't think a requirement is the way to go. I get sucked in every now and then through the build team and think that works great. If we could improve notifications/pings that would be my preferred outcome of this issue.
orangemocha commentedon Apr 14, 2016
As the author of a recent PR that broke streams, I can attest to the fact that the code is hard to understand, and seems somewhat fragile. More tests would certainly help avoid regressions. Please also consider documenting the internals (e.g. the state machine) so that more people can understand and hopefully improve this code.
MylesBorins commentedon Apr 14, 2016
I would like to suggest a
Streams SummitThis could potentially be somewhat similar to the summits that @jasnell has organized in the past.
I for one would love to get more involved, but find that I don't exactly know where to start, and am a bit overwhelmed with the entire system.
Arranging a summit that can be part knowledge transfer and part project co-ordination could help move us in the right direction to making things more stable.
@nodejs/tsc is this something that ya'll might get behind?
mcollina commentedon Apr 14, 2016
I'm really 👍 with the idea of a Stream Summit, not sure how the organization and logistic will work.
Fishrock123 commentedon Apr 14, 2016
I think @chrisdickinson has had the best suggestion in the past, something along the lines of documenting the entirety of the streams interface, as a base to start from.
Not sure if this should be done before, or as a Summit.
mcollina commentedon Apr 20, 2016
@Fishrock123 I don't think it's possible to do it as a Summit. documenting the whole internals is a long process. I think we can kickstart that by doing short presentations and record them on the major areas, mainly to set a good base to discuss the future of those. It would be a good task for a new entry in the wg, mainly because we are all too biased to explain it in a way that's understandable (given the fact that we all learned it reading the code).
Maybe @nodejs/ctc can help setting this up? @rvagg?
MylesBorins commentedon Apr 20, 2016
I'm open to helping lead the organization of an event like this. I had the chance to talk to @rvagg about it and he seemed positive about the possibility. I've opened a new issue up on the TSC repo to start the discussion more officially nodejs/TSC#93
jasnell commentedon Apr 22, 2016
So the planning for having a new collaborators summit around the Interactive events is under way and it should include at least some time for the WG's to get together. On this particular issue, is there reason to keep this open?
Perhaps @Fishrock123 could use his experimental bot to perform automatic mentioning of the streams wg if the issue is tagged as
stream?phillipj commentedon Apr 22, 2016
@jasnell absolutely, that's a trivial feature as soon as we're satisfied with the auto PR labelling test we're doing nowadays.
Refs #6247
jasnell commentedon May 30, 2017
Closing this as I believe there's nothing else to do. Generally speaking, if it touches streams, someone from @nodejs/streams should sign off.