On Thu, Feb 13, 2020 at 10:19:28AM -0500, Brian Foster wrote: > I agree with just about all of this mail, but I do have a question for > Darrick and Dave.. in what ways do we (XFS community) place the burden > on the maintainer to garner/coordinate review? I get the impression that > some of this is implied or unintentional simply by reviews coming in > slow or whatever, and the maintainer feeling the need to fill the gap. I That's pretty much it. If nobody else reviews the code, if has fallen to the maintainer to review it so that it can be merged. i.e. review is needed to make forwards progress, to get stuff merged we need review, but if nobody reviews the code for weeks, it essentially falls to the maintainer to do a review to determine if the patchset should be merged or not. > can sort of understand that behavior as being human nature, but OTOH > there's only so many developers with so much time available for review. Yet all those developers seem to be happen to write code under the assumption that someone will review it.... > That's not generally something we can control. Given that, I don't think > it's necessarily useful to say that review throughput is the fundamental > problem vs. this unfortunate dynamic of limited reviews not meeting > demand leading to excess maintainer pressure. I'm trying to think about > how we can deal with this more fundamentally beyond just saying "do more > review" or having some annoying ownership process, but I think this > requires more input from you guys who have experienced it... /me rocks back and forth on his rocking chair.... Back in the days of being an SGI engineer working on IRIX - which is where this process of review came from - each 3 monthly OS release had a couple of "release tech leads" whose only responsibility for those entire 3 months of the release was to review and accept code into the Irix tree from the entire of engineering. Every 3 month release had different engineers performing the release lead role. It was not a fixed position, and when you were assigned this role you were given leave from your team responsibilities. i.e. the release lead performed duties for the entire engineering department, not just a single team. Nothing got to the release leads without first having been peer reviewed, unit tested and signed off by the reviewer. It was the responsibility of the person who wrote the code to find a reviewer and to get the review done, and only then could they tick the box in the bug system that said "here's the change, here's the tree to pull from, please review for inclusion". That's when the release lead is notified that a change is ready to be merged. IOWs, the release lead only ever saw reviewed, tested, completed code ready for merge. Everything else was the responsibility of the engineer tasks with implementing the change/fix via the bug system. The release lead would check all the commit metadata was correct, that the code merged cleanly, looked sane and, finally, passed basic integration tests. They typically could not do more review than this because most of what they looked at was outside of their specific area of expertise. Once the the release lead approved the change, the engineer would then run the "commit script" which would check approvals in the bug system and then merge the code into the main tree (i.e. the release lead didn't actually do the commits themselves, just approved them). We've always held that the position of "XFS Maintainer" was effectively that of a "Release Lead" - someone who checks over reviewed, tested, completed code, except with the added requirement that they maintain the tree that everyone works from and sends requests for the tree to be merged with upstream. That is, organising review and/or reviewing every patch at great detail was completely outside the scope of the Release Leads' role. They look for integration and merge issues, not the fine details of how a feature or fix is implemented. IOWs, the Release Lead role is one of high level staging co-ordination, testing, oversight and tree management. Now, compare that to the traditional role of a Lead Developer. A LD tends to spend a lot of their time on knowledge transfer, review, architecture and project direction. The LD typically only looks at code in their domain of expertise, and works with other LDs at architectural/design levels. But when the shit hits the fan, it's the LD who digs in to that problem and does what is necessary to Get Stuff Done or find that difficult bug. Hence if nobody is reviewing code and that is creating a backlog, then the LD has a responsibility to the team to make sure that review gets done in a timely fashion so code meets the requirement for merging. This is a very different role to a release lead role. The problem is that Linux Maintainer role combines the roles of "Lead Developer" with "Release Lead". i.e. the person who has the release lead role for maintaining the tree for the team is also expected to perform the LD role, and that means they are also the "reviewer of last resort". Hence if no-one else reviews code, it comes down to the maintainer to do that review. IOWs, the Linux Maintainer has the responsibility to the team to keep making forwards progress by merging code, but they also have the responsibility to be the person who reviews code when nobody else does. Rather than a separation of these roles, it makes the assumption that someone who is a good enough engineer to reach a senior position in a team is also proficient at managing source trees, automating testing, dealing with upstream merges. Making your best developer also responsible for doing largely the same thing over and over again (merge, test, push) is not a good use of their time. Often it's a step too far - talented engineers often only want to build things and don't respond well to being asked to do mundane repetitive tasks. It's kinda like asking an areospace engineer who designs planes to also be the mechanic that makes sure every bolt on every plane is tight and won't come undone during flight... In reality, anyone who can use git and has a decent understanding of the code base, team rules and git workflow can perform the Release Lead role. But a fair few people have said they don't want to do it because they are scared of making a mistake and being yelled at by Our Mighty Leaders. That is a result of the fact that a Linux Maintainer is seen as a _powerful position_ because of the _control and influence_ it gives that person. It's also treated like an exclusive club (e.g. invite-only conferences for Maintainers) and it's effectively a "job for life". i.e. Once people get to this level, they don't want to step away from the role even if they are bad at it or it stresses them out severely. How many people do you know who have voluntarily given up a Maintainer position because they really didn't want to do it or they thought someone else could do a better job? Personally, I never wanted to be a Maintainer. For a long time I was simply a "Lead Developer" and through no choice of my own I ended up as the community elected Maintainer. Performing the roles of LD + TL for XFS kernel, xfsprogs and xfstests was too much for me - I dislike the mechanical, repetitive process of managing patches and branches for a release and when combined with doing review for so many different things, it eventually that burnt the care-factor out of me. Which brings us to today - when I stepped back, we ended up with separate maintainers for kernel, progs and fstests. fstests is relatively low volume compared to the XFS kernel and xfsprogs so that's been fine. However, we're seeing that the maintainers of both of the XFS trees are showing signs of the stress of the responsibility of maintaining both the trees, the reviewer of last resort, doing all the integration testing and trying to keep up with all the craziness that is happening throughout the rest of the kernel. So, really, it sounds like splitting what I was doing into 3 separate Maintainers hasn't really been that successful. It hasn't solved the burnout issues, but we've known that for quite some time. The concept of a rotating Maintainer role was born out of the idea that we'd move it back closer to a Release Lead role and separate it from the reviewer of last resort problem. We haven't really got past talking about it, though, so perhaps we need to actually do something about it. I know this probably doesn't really answer your question, Brian, but I thought it might be useful to given some further background on how/why we have the review processes we do for XFS, and where the maintainer has tended to fit in to the picture. Up until I was elected maintainer, we'd kept the role of release lead separate to the role of lead developer and we didn't have a burnout problem. It certainly seems to me like we took a step in the wrong direction by trying to conform to the Linux norms for selecting our most productive developers to perform the official Linux Maintainer role.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
