> I disagree with Roman's suggestions. If a PR has enough votes, we should > trust the committers approving the PR and move forward.
FWIW, I do think it's good to be courteous and give other reviewers a day or two to either follow up on a review or decide to leave the decision to the reviewers that have already chimed in. I just think that allowing two weeks for that is excessive and would lead to PRs languishing in the queue even more than they do now. On Mon, Jan 28, 2019 at 1:30 PM Fangjin Yang <fang...@imply.io> wrote: > 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. > > > > > >
