Security bugs should go through PSRT. This will ensure that release managers are notified on security bugs. Security fixes should be thoroughly reviewed by multiple people, too. You want to ensure that a fix closes the bug and not introduce a new one (which has happened before).
Carefully smashed, though.
What about the case of a topic area that has experts listed, but the experts haven’t been responding to other issues in their area for some weeks or months? If the experts already haven’t been responding to other issues, “resetting the clock” on new issues doesn’t seem right to me.
Note: the Experts Index page says:
If a listed maintainer does not respond to requests for comment for an extended period (three weeks or more), they should be marked as inactive in this list by placing the word ‘inactive’ in parenthesis behind their tracker id. They are of course free to remove that inactive mark at any time.
But I’m not sure this is done in practice. I for one wouldn’t feel comfortable marking someone “inactive.”
On that note, I am on the “experts” list in a number of places, but it’s very hard for me to see notifications when there’s stuff for me to review - the overall github and bpo traffic is way too high, so I have everything muted. Ideally I’d like to find a way to just get notifications on things that I’m pinged on, or similar, but I’ve not had time to look at how to do that (I get nosied on every Windows issue at the moment, for example, which isn’t particularly helpful - although being made aware of a subset of Windows issues might well make sense).
All of which is to say that “inactive” is a bit strong, but “difficult to get my attention” is true - but it’s not because I don’t want to review stuff, though, as much as I haven’t worked out how to deal with the tooling 
I don’t know if my experience is unusual, but making it easier for people to find things their input is needed/appropriate on might be an alternative way of increasing throughput on reviews.
1 Like
Fair enough, @pf_moore. In my comment I was thinking more of experts that haven’t really been responding on any issue (e.g. taking an extended break). FWIW, I agree that if an expert is active on some issues, they shouldn’t be marked inactive (i.e. missing one issue shouldn’t be a reason for being marked “inactive”).
While this doesn’t directly address your requirements, a poorly advertised feature of github is https://github.com/pulls/review-requested - you’ll need to remember to check it periodically, but it’s valuable when you do.
Mark
1 Like
Thanks, I didn’t know about that.
Hmm, sadly it looks like there’s still a load of “review requested” on there because I’m part of the “Windows team”. I suspect the real issue here is simply that “Windows team” is a bit too broad - it gets added for everything that even touches a file that’s deemed as “Windows related”. Or maybe I should just not be part of that team (although that seems counter-productive 
)
1 Like
It’s too noisy for me as well, but once we’re both off the team then there’s really no value in having it there at all 
BPO I can handle, I think because it reads like a mailing list. Github doesn’t come close, but hopefully our incredible triage team will keep being amazing!
Keeping the experts list up to date is a good way to help them out, but at the same time, empty entries don’t help anyone. That’s the place where someone with paid time could help out - using that time to become enough of an expert in any unstaffed area to deal with whatever issues are coming in. (Though I’d still rather “assign” someone than rely on a fallback too much.)
1 Like
Personally, I would greatly prefer to have an empty entry rather than one that lists only inactive expert(s) which haven’t yet been marked as “inactive” or replied to any issue/PR for an extended period of time. It at least helps to indicate what areas we currently lack experts in, and from a triaging perspective, helps to indicate who is worth pinging for a review.
In my experience, it can be rather discouraging for the author of a PR to repeatedly see pings and requested reviews, only for them to not be answered.
2 Likes
This is a good idea however am not sure about the practicality in implementation. At a time where we have more PRs than reviews. It leads to alot of stall for even the simplest of PRs e.g a simple typo can take months to ever be seen by an expert of the module.
An empty entry is still going to cause this, which was my point. But an entry that has active people listed in it is even better, and we should be striving to replace experts rather than just clear them out.
1 Like
My point was that there’s also a communication issue - lack of a response to pings may indeed mean a core dev who’s no longer active. But it may also indicate a core dev who isn’t able to see pings.
I try to respond to any zipapp and packaging issues that get raised, but it’s hard to see them in among the other traffic I get that isn’t relevant to me. I currently have a bpo email about a zipapp issue pinned in my email client, and I have to remember to keep following the link in that to a github issue, to check for updates, because none of the “normal” ping mechanisms will reach me among the bulk of pings that I ignore as not relevant to me.
(This seems like it’s drifting off topic. Maybe the discussion should be split so that discussions on notifications and pings aren’t getting in the way of the main thread here?)
2 Likes
Sometimes I see the “expert list” and “code owners” as gate keepers: a PR should not pass unless they approve it. In theory, it’s a great idea to review PRs. In my experience, it’s quite hard to get a review in less than one week. Sometimes, if I don’t get a review in less than one week, it usually means that I will never get any review on this PR.
I would love to get a review on all of my PRs, but I abandoned this idea long time ago. I defined some guidelines for myself to decide if I must wait for at least one review for a PR or not.
For example, I really dislike touching ceval.c or gcmodule.c without a review since I’m not comfortable in this part of the code, and it’s easy to mess up when touching them. In general, I also prefer to get a review when I add a new public function (at the C level or Python level).
For refactoring-only changes which don’t change the behavior in any way, I just go ahead without review. Development velocity also matters for me: since GitHub doesn’t support “patch series” (multiple chained PR), I like to merge many PRs in a row to get small “atomic” commits. Having to wait for the CI around 30 min to push a single commit is already very long. Pushing 10 small commits takes me 5 hours. Having to wait for a review which takes 1 week for a serie of 10 commits would take 10 weeks instead of 5 hours…
To get reviews, sometimes I ping some developers on IRC, Google Hangout, WhatsApp, IRC, etc. Whatever works 
Sometimes I wrote an email. In my experience, most core developers (if not all) ignore all GitHub notifications.
I didn’t write “review by core developer” on purpose here. In the last 12 months, I got more and more often reviews from contributors who are not core developers. It makes me feel more comfortable to get a review from a complete foreigner to me, than having no review at all!
I dislike pinging core developers too often, so I either try to not propose too many pull requests in parallel, or I skip the review step.
What worked well for me is to work in pair. I did that sometimes with Pablo and Dong-hee for some specific changes. Text chat or video call, whatever works It’s also a way to get more eyes on your code without going through to “formal” review process. Usually, we spent most of the time on investigating a bug or discussing options to solve an issue, rather than writing the code. Reviews can be done later “off-line”
7 Likes
I like this idea. It could be interesting to expand the office hours concept that we already have for newer contributors to also include pair/group programming between core devs. I’m game to try it if anyone wants, at least 
.
Tangentially, I’d also encourage others to try setting up an office hour of some sort; I haven’t had a huge number of takers (I think I can count the meetings in the past two years on both hands without resorting to binary), but I think it’s gone reasonably well when it’s happened.
3 Likes