I don't think it's irresponsible to start a review and not be able to finish it promptly. But drawing the process out can feel frustrating to other committers that have already finished reviewing, like they are being told that their review is not good enough. I think it's a matter of degree. Two weeks sounds very long to me but two or three days sounds reasonable. Part of the rationale for this is that PR review is an iterative process. If each iteration could take two weeks then a patch might not be committed for months. (This happens sometimes, and it is sad, and sometimes I've been on the guilty end of it, and it's something I think we should try to avoid by endeavoring to speed up review cycles.)
It's a totally different situation if nobody else has reviewed a patch yet. In that case a reviewer reviewing things with longer cycles isn't blocking anything. They are actually helping a lot by being the only person willing to review the patch at all. In this case I think the etiquette and timings you suggested are more reasonable. I guess that reviewers prioritize the newest PRs first because of how the GitHub UI works. By default it sorts PRs by created date, newest first. And it doesn't have an option to sort by "most time elapsed since review". Maybe we should make our own review console that sorts the PRs differently? Or shoot for PR-zero (like inbox-zero): close all PRs that haven't had comments in 6 months and try to review all the others as fast as possible. On Mon, Jan 28, 2019 at 8:43 AM Roman Leventov <leventov...@gmail.com> wrote: > On Fri, 25 Jan 2019 at 23:12, Gian Merlino <g...@apache.org> wrote: > > > If enough other committers have already reviewed and accepted a patch, I > > don't think it's fair to the author or to those other reviewers for > > committing to be delayed by two weeks because another committer doesn't > > have time to finish reviewing, but wants others to wait for them anyway. > > > It sounds like it's implied that the reviewer is irresponsible because he > started reviewing a PR but "doesn't have time to finish reviewing". In > fact, a reviewer could *have* time to finish reviewing and is willing to > finish the review, but they don't have time *tomorrow*. A reviewer could > have different duties and could slice only Y hours for reviews of Druid PRs > every X days. X/(Y * number of PRs the reviewer is interested in at the > moment) is how often (in days) a reviewer could pay attention to specific > PR. I think we should respect that for some people, at least at some times, > this value could grow to about 7. Saying that we shouldn't wait for those > people creates a bias favoring most involved developers, and it's not > necessarily a good bias, because sometimes outsider (or half-outsider) > perspective is valuable. > > If we do releases every 3 months and the time between creating a release > branch and the final release candidate is at least 3 weeks (historically), > why there is an urge to commit anything faster. > > IMO the real source of unfairness in reviews is that reviewers generally > prioritize the newest PRs rather than PRs that await for reviews the > longest. The probability that somebody starts to review a PR decreases with > time, while it should increase. >
