I disagree with Roman's suggestions. If a PR has enough votes, we should trust the committers approving the PR and move forward.
On Mon, Jan 28, 2019 at 9:28 AM Gian Merlino <g...@apache.org> wrote: > 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. > > >
