-
Notifications
You must be signed in to change notification settings - Fork 56
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
Project Velocity and PR reviews #64
Comments
Lots to discuss there, and I'm definitely not the best to discuss most of it, but I'm deeply interested in hearing/reading discussions about this. Burnout is a very real problem, and I feel it's even worse in OSS since we're supposed to do this because we love it. TL;DR: huge +1 on discussing this at Summit. I'll leave the rest to competent people. |
So, here is actionable stuff that might encourage reviews (some from the post above):
|
Great idea to bring this up at the summit 👍 |
I'll throw in my two cents again since some of this is pretty much like things currently in discussions in the CommComm restructuring effort.
This is precisely the point, at least in the CommComm. We've noticed people tend to think you have to join the CommComm/contribute in the CommComm repo to be a "CommComm contributor". I think there may be a bit of the same thing at work here, and that may prove to be an org-wide problem. We may have to improve the recognition of work done in the subteams as "real contribution". Anyway, real discussion material there 😅 |
+1, several good ideas suggested already and definitely a good topic for the summit. |
Another thing we can suggest is letting teams decide on a tracking policy. That way teams can take longer than 72h (over weekends) or less (fast-stack) to land PRs. Some use cases:
All such request would be part of the monthly (by monthly) interaction of the team with the TSC who'd approve/vote on them. |
A hard +1 for stronger, more independent subsystem teams. We should definitely encourage people to join any subsystem teams they're interested in working with, but IMHO we'd need to properly incentivize in order to make it more successful. This could be easily done by giving the subsystem teams perhaps a little more autonomy (they're still very much answerable to the TSC, more on below). This could be done by allowing the subsystem teams to have atleast a bi-weekly meeting where they triage relevant issues and discuss PRs. Only members of the concerned subsystem team would land a given PR, making sure it aligns well with the current position of the team. Each team would send a single member to represent them in TSC meetings (we could rotate people or elect them, may vary from team to team) who would report the TSC on the current status of the subsystem and take directions in case they need some. How does that sound? |
Considering that this would be a short presentation and a workshop at the summit, it would be fantastic if in the presentation you sum up the current pain points and the goals for the workshop session. I think it would be very nice if at the end of it we can come up with some process recommendation for the TSC to adopt and iterate on. |
I'm strongly -1 on this approach. It will just add an extra thing to keep track of on top of the many PRs, and create an in-group of those who can afford to attend 3+ meetings every two weeks — adding to many collaborators already busy schedules. As someone that contributes to 3 teams regularly and another 2-3 fairly often, I would just have to quit any paid work and do open source full time. |
I agree with @apapirovski above. More generally, when trying to address any issues (not just this one) at the summit, it would be best to avoid if at all possible:
Now that I've made my "don't" list, here's my "do" list of things to try to do whenever feasible:
These are guidelines and not hard rules. (For example, I'm much more open to new procedures if we are emulating another project that has already solved a problem we are trying to solve.) |
Possible things that might help with this specific issue:
|
I wish there was a more formal way to conduct and record the result of polls. The current ways seems to either be reactions (from which you can't tell if someone changes their vote after a decision has been reached) or tallying comments (which adds lots of noise to the PR/issue). I'm thinking here about approvals for fast tracking, but this could equally be useful for TSC votes.
Labels? |
I also prefer not adding something like a biweekly meeting as it would require a lot more engagement. I was thinking giving the # of meetings to each with teams presenting to the TSC once every two months. I agree about automation @Trott, the idea of making teams more autonomous is to do less nodejs/collaborators pings - in general I think we should aim to greatly reduce the amount of churn and things someone has to read in order to contribute. That was the point of #64 (comment) at least.
I don't think we're there yet - I think we should discuss and decide how many notifications people would expect. Stuff that is not urgent can be digested and pulled instead of pushed - stuff that is (like stuff stalling PRs) should be pushed and there should be an obvious way to tell the difference.
👍
That would help - but I think this is more of a cultural decision of when to ping who. |
Can we make github-bot visit stalled PRs and cc focused teams according to labels? |
@vsemozhetbyt that would trigger more pings overall - I think there we should be aiming for more focused pings rather than more pings. |
@vsemozhetbyt The CODEOWNERS file can do that https://help.github.com/articles/about-codeowners/ but I am not sure pinging teams helps the issue or simply makes it worse |
Interesting, it looks like we could use this to enforce things like nodejs/node#6178? I wouldn't want to do it across all of core, but it might make sense for things such as streams. |
This is a very good idea @vsemozhetbyt
What do you think? |
Here's the problem that I think needs to be addressed: There are people in leadership positions in the project who don't have sufficient time to review all the things that they have a stake in and need to care about. AFAICT, the problem is not "people aren't getting pinged" or "people aren't getting pinged enough" or "we aren't moving fast enough". On the contrary, people seem to be ignoring notifications because they're already overwhelming. Is the fact that some PRs stall a problem? Maybe. Do some PRs take too long to get adequate review? Sometimes. But those are (IMO) mere nuisances compared to the after-the-fact "What? When did this happen? This needs to be reverted!" from Someone Everyone Else Listens To. I think the problems here are very difficult:
|
I don't feel 100% on these suggestions but I suspect they're worth at least considering:
|
I'd like just to say that I completely agree with @Trott :
The reason I suggested team pings is in order to greatly reduce overall pings and removing most of nodejs/collaborators pings. I think that if we make interaction less overwhelming people would naturally be more available and willing to engage in the project. |
Some of the answers which I can think of:
Encourage companies who use Node.js to employ some developers to work full-time or part-time on Node core. We should brainstorm on how to incentivize these companies to contribute back to Open Source.
May be we should revisit why there's so much code in a single repository, and split it to multiple ones? I just became a collaborator last month, so don't know the history on why these decisions were taken. |
On the subject of incentives, the majority of the node collabs are not paid to be here and as much as we all enjoy programming i think being a node collab sometimes goes beyond enjoyment and into "work." maybe we can look into getting some small incentives for collabs such as time on hosting services for x prs reviewed or something like that. Going back to the "work" aspect, i think we should also think about ways to make contributing to node more enjoyable, maybe even fun. I'm here just 'cause i enjoy working on the code but getting a change into node is not what i would describe as an enjoyable process. Over time node seems to becoming more and more fixed and official and business-y which is fine 'cause we're not exactly a small project but i think we need to find ways to help reduce the stress of the process. |
Honestly anything financial would not incentivize me (it would mostly just take the fun out of it). The only financial instrument that actually helps (IMO) is the travel fund which I personally am going to use this May for the first time in order to attend the summit.
I think that a significant amount of discussion in the CC is geared towards that. It's a broader overall issue. |
This is only somewhat related but I think it's worth considering having formal stretches of time where we dedicate all collaborator time to fixing issues from the tracker rather than adding new features / refactoring. Perhaps these could take a form of an online mini-summit where people can also pair up based on their interests and areas of expertise. Debugging and/or fixing some of these edge case issues isn't super fun (especially when it's some obscure single platform specific issue) but I think having collaborators working together to do so might be quite engaging and enable people to learn something new. We're at 593 issues right now and I don't think there are that many left that can easily be closed down without providing an actual fix. That's pretty intimidating and overwhelming. I don't think we'll get to 0 but I think everyone would feel like they have a slightly better handle on the project if we got it down to 200-300. |
This is an important thing to talk about from time to time in general. For this specific issue, here's the thing: The people we're talking about already get paid to work on Node.js. Aside: The "get more companies to pay people to work on the project" thing is more complicated than it may appear at first, even putting aside the practical challenges of persuading companies to donate significant resources for the common good. I don't want a project where the only way you can contribute more than marginally is with the support of a business entity. On the other hand, I also don't want a project where big companies benefit from free volunteer labor without giving back. How do you improve the balance from where it is now? (It's not an easy question. Our current coexistence of volunteers, partially-paid-by-their-employers folks, a small number of fully-paid-by-their-employers folks, and corporate foundation membership providing financial resources...it's a good setup compared to some of the easily-imaginable alternatives.)
Just in case this isn't obvious: There is nothing stopping you from forming a group like that. If it's something you'd love, consider trying to make it happen. Groups often form ad hoc to do work and only get officially chartered at a later date, or don't even bother getting officially chartered at all. If you would love a team like that, you can make it happen as long as you're willing to do the work to get the team up and running and keep it running. (That can be the hard part!) If you're super-motivated to make it happen, maybe open a discussion board topic to find like-minded folks and/or invite like-minded folks if you already know some. |
Precisely, I can name a project or two that realistically work that way (think browsers) and I don't think that we want to follow that path.
I'd like to emphasize this point - there was lot of interesting discussion in teams that never made it to working groups. Chartering is only important if you need to make decisions autonomously - it's typically better to go through the TSC and get feedback. |
Ping @nodejs/collaborators - I'd like to work on this this weekend and I would like to solicit opinions. I'm planning for this to be mostly a discussion where we hear out the thoughts collaborators have on this issue and discuss what to optimize for. I am not aiming to present solutions yet. My hope is to work on something to present to the TSC and CommComm after the summit that would be actionable. |
Thanks a lot for that! We'll discuss this in the summit.
I'm definitely in the camp of "these help me improve" and I enjoy reading nitpicks, but maybe that's just me. |
I definitely think using part of the time at the summit to discuss what we do when the CI is unstable is important. I think we need some criteria (not sure exactly what) that when hit (or maybe a process through which collaborators can request it) we say "stop" and focus on getting the CI back to green. |
We might consider looking how other OSS projects handle this. |
regarding CI and test fail traceability - most subjobs have been moved to a better Jenkins test parsing plugin, so the need to check console output should be much reduced. |
@refack it's definitely a nice start! What do we need to do to have this on all BTW, @joyeecheung has a PR to node-core-utils to make traceability easier too: nodejs/node-core-utils#161 |
We could have two criterias ¹ ²
Again, we're talking about the project "moving too fast", there's no problem in slowing it down in exchange for a more stable CI. It would be nice if marking a test as flaky was smoother (today we need to open a PR, get approvals + usually fast-track approvals, then go through the landing process). ¹ Both criterias should be enforced by a tool (for example, GitHub interface). We shouldn't have to rely on a policy to enforce them, because it won't work (waiting for 2 approvals to fast-track a PRs sometimes is ignores, for example) |
This can ties directly into improving releases and LTS
Both for velocity (lots of stuff landing on master and few people
backporting) and automation (a bot automating backpprts)
…On Wed, May 30, 2018, 4:10 PM Matheus Marchini ***@***.***> wrote:
I think we need some criteria (not sure exactly what) that when hit (or
maybe a process through which collaborators can request it) we say "stop"
and focus on getting the CI back to green
We could have two criterias ¹ ²
- If the CI is red, don't land a PR until it's green or yellow
- If we identify a flaky test during an unrelated PR, either add a
flaky status to it or fix it (if it's an easy fix)
- If it's an infra issue, wait until it's fixed.
- When we reach a certain flakiness treshold (let's say, 20 known
flaky tests), block everything from landing until we get back to < 20.
Again, we're talking about the project "moving too fast", there's no
problem in slowing it down in exchange for a more stable CI.
It would be nice if marking a test as flaky was smoother (today we need to
open a PR, get approvals + usually fast-track approvals, then go through
the landing process).
¹ Both criterias should be enforced by a tool (for example, GitHub
interface). We shouldn't have to rely on a policy to enforce them, because
it won't work (waiting for 2 approvals to fast-track a PRs sometimes is
ignores, for example)
² We might have to consider a lighter policy for security patches.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#64 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAecV06WdFgwVK0uuo4SCuDt-lozaVfkks5t3qhlgaJpZM4Tbn6X>
.
|
I believe we briefly discussed this at the last summit in October, a commit queue sounds good in theory and thanks to @joyeecheung and many other great people, we now have |
Something we should discuss during the collaboration summi
|
Another idea (I am not sure how we will make an agenda for this session so I will just log it here in case I forgot): we should make a repo for the flaky tests - right now the related issues are scattered in build and the core. We need some effort focusing on this, some records tracking the flakiness maybe, that should happen in a dedicated space to reduce the noise. |
@joyeecheung I agree, although we might need to think how to handle tests we think are flaky but are actually highlighting a bug/design flaw in our code (see nodejs/node#21012 (comment) for example). Maybe if we identify there's an actual bug in place, we should move the issue from the flaky-tracking repo to nodejs/node? |
@joyeecheung leaving a comment here works, I want to go through the suggestions in this issue and discuss them during the session. |
@mmarchini SGTM. The repo is there to reduce the noise and it would be easier for people to quickly understand what's going on in the CI in case they have not checked it out for a while. |
Do we really need a separate repo? Why not labels and/or a project? |
@richardlau Because being scattered around in the core repo makes them harder to find (the flaky label is only applied to flaky tests, not build flakes). Also a lot of issues are build issues or something combined, it would be easier to search for relevant failures if the issues are in one repo when you see failures in the CI and don't know where to start (I search in my emails but not everybody subscribes to both nodejs/node and nodejs/build) |
Also the flaky test issues are making the core repo even more noisy, they could be easier to work with if we deal with those in a separate repo that people can actually subscribe to. |
https://github.com/nodejs/node/projects/8 already exists. My concerns with a separate repository is that it reinforces the idea that flaky tests are someone else's problem to fix. |
@richardlau I am afraid people who don't work on flaky tests don't work on them already (I sometimes work on those but when I don't have the time to specifically work on flakes and stop following, I am completely ignorant about what's going on in the CI). The project reminds me that if we have a separate repo for the issue, we could apply different sets of labels for them and find relevant failures - e.g. some tests crash in a similar way, some tests are sensitive to GC and timing, etc. Being able to find the relationship would help fixing those if we have difficulty reproducing flakes. |
Speaker queue: https://tcq.app/meeting/6tS3 |
@benjamingr have a look at Review Board open source code review https://www.reviewboard.org/ |
Is there a minute or list of actions from this session? |
@mmarchini Not really. (Someone correct me if I'm wrong, though!) I think it's one of the downsides of the plenary sessions. They tend to generate a lot of conversation but not much in terms of ongoing action the way you get with small focused groups working on problems. In the afternoon, we had a related break-out session on CI and came up with these action items:
|
Thanks @Trott! I'm interested in participating more in the Build WG (currently participating in a few issues, working on a CI Job Health Dashboard and llnode jobs on Jenkins (not sure we still want to pursue this last one though). I'm also interested in helping the Commit Queue Prototyping Team, if possible. |
Would be nice to have a combined stress single test job with a git blame-like feature to find out the lkgr for a given flaky test. |
Following an internal discussion about concerns of high project velocity and recent PRs landing and then being reverted, I'm copying my response publicly here (below).
I'd like to discuss what we can do to improve the PR process this during the summit if anyone is interested.
Thinking a little more about this - I definitely think it's a matter of not getting enough eyes to review the code rather than bigger changes.
I think this is also very discouraging to a collaborator working on a PR because:
I think that in order to improve the # of eyes on PRs we have two strategies:
I think we should work towards a more organized review system where:
Optimally, a collaborator joining would automatically join the teams related to the PRs said collaborator worked on when joining - we should also likely ping existing collaborators.
Would it make sense? It's really just broad ideas.
The text was updated successfully, but these errors were encountered: