This is a very interesting idea. I like the idea of recognizing people who are putting in hard work of doing reviews but are not official code owners. This would also help define the process of how code owners provide mentorship.
I believe this could help, but also think we need to be encouraging folks to step into these roles and not rely on self nomination. I also think sometimes its hard to nominate outside your normal network for whatever reason. It would be good to design a process to make sure we are looking at all available candidates at each step. Perhaps having an intermediate position would be helpful for that too.
Maybe it’s as simple as explaining what to do in these situations and follow a similar procedure.
I think you are mis-understanding what I am saying. I’m asking for more transparency and people who are overseeing the process with the goal of transparency and fairness. I’m not talking about making a group of “king-makers” (or queen-makers). We already have a situation that is “who you know” in order to be a code owner or nominating mostly within their organizations or networks. I want LLVM be balanced when it comes to all interests involved. So I’m suggesting we think about the process a bit and brainstorm how we can achieve that end goal.
FWIW, I love the discussion on this thread - we clearly need to evolve here. To +1 a few random points:
I don’t think it is a bad thing to have a hierarchy of reviewers. This provides a natural escalation path if two co-owners have a disagreement and want a new opinion/tiebreaker etc. The codebase is hierarchical already, and overall owners gives coverage to things that otherwise fall through the cracks.
I hope that massive disagreements will be rare, but if they come up, the LLVM decision making process is the established thing for resolving technical discussions in a visible and inclusive way. I consider it a feature that it doesn’t get used very often, but we have it when/if we need it.
I love the idea of formalizing more of our process and intent; codifying the status quo we like and defining new things in a written way. This is far better for newcomers who haven’t been part of the community for a long time, and I really agree it would reduce some of the intimidation factor that could lead to folks not raising their hand.
I really really love to idea of giving more visibility and recognition to contributors to LLVM of all sorts. llvm/CREDITS.TXT only has 115 people listed in it which is laughable. Maybe there are other ways we can give kudos to contributors, e.g. in the llvm release notes, at developer meetings, etc.
@AaronBallman a slight aside on this ownership conversation here are some things I struggle with for us to all think about… (re Code Reviews)
As you know I help out on clang-format, And whilst I appreciate the recognition of being considered an owner, my bigger concern is:
defect backlog (which has grown significantly since the move to github issues), we improved our defect logging speed/capability but did nothing to increase our fixing speed or increased the number of volunteers significantly enough to keep up (although we did pick up on or two more)
We already had 4 key owners in my view (@owenpan , @HazardyKnusperkeks , @mkurdej and myself @MyDeveloperDay), we work well (in my view) as an informal leadership team. I have a lot of respect for these contributors (+ a few key contributors who jump in from time to time). But to be honest we get a lot of contributions from people outside this team which is great but people drive by with a patch, hang around for the first couple of rounds of review then I believe lose the will to live! very few reviews make it past a couple of rounds. Our core team wastes significant amount of their time on these wasted review (to the extent that I find it difficult to find the time to also fix bugs or add new features myself)
This leave us with a massive review backlog (in Needs revision), for which I never see the work ever finished, we often give significant amounts of time to reviews which end up going silent. - example D54628 (last updated 2019)
When ultimately we review back with either rejection or significant changes the contributor walks away leaving the review is a floating “Needs Review/Needs Revision/Changes Planned” state.
But dead reviews often the author is radio silent (won’t respond to ping), we can’t get them to Abandon.
Abandoning a review requires “taking ownership” which makes you look like the author of the review, I don’t want that…I just want to mark them as “dead”, this also feels a bit rude…
We need a much more proactive “Abandonment” policy/mechanism to flush these items out of the system or the reviewers are constantly having to look at these dead items in their lists of what needs reviewing. (can’t tell you how many times I’ve relooked at D33029 - 5 years old)
I’d like to see an automated “Grimreaper” that kills inactive reviews after N months of inactivity. (its not deletion the owner can reopen), I want it to be a “bot” so we depersonalise the “Abandonment”, i.e. it wasn’t the owner that Abandoned it but the author because they didn’t do anything with it.
Any thoughts on setting a “What to expect will happen to your review” policy…
This doesn’t particularly add to the topic of ownership or reviews, but I’d like to just say as someone who only just made a patch, specifically one in clang-format
I did so because it is probably the “lowest stress” subcomponent of the LLVM monolith, and perhaps with the lowest barrier of entry, not requiring prior extensive study of compilers/codegen/optimizations.
I’m sure you know this, which is why you get lots of outsiders (i.e. those with no commit access, such as myself)
Nevertheless, I had a very enjoyable experience and greatly admire the team of 4 aforementioned reviewers (this is really a thank you letter). So I can’t comment on why people “lose the will to live”
I’d also like to mention that when I first read in the contribution guide the daunting task to “find reviewers”, the code owners file wasn’t much use (glad it’s been improved!), but the method I used which isn’t mentioned in the guide was to look for prior, recent accepted reviews in the same area and just copy the reviewers listed there. (“If other people are doing it, surely it must be okay”)
Perhaps this doesn’t apply to all parts of the codebase because I’ve really only looked at one, but it seems to me that using git blame could result in finding people that are no longer active, etc.
Do you have any insight on why you think folks seem to walk away after a few rounds? Like, is it a matter of folks not realizing how much effort is involved and once they find out, they feel like they’re in over their heads and walk away? Or is there something else going on that we should think about addressing so that we retain these folks?
This is an interesting idea, especially if we switch to GitHub (where I believe bots are easier to support than Phab). In general, I think it makes sense to automate this process – the trick will be in finding a good timeframe in which to run it. e.g., reviewers or patch authors may go on vacation and we don’t want the bot to come by and claim something is abandoned when it actually isn’t. But if we were okay with something like six months of no activity, I have a hard time imagining the bot causing hard feelings.
