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. A couple of days, sure, but two weeks is quite a long time. It would be potentially longer in practice, since with your proposal the two-week clock would start fresh if the reviewer had some more follow-up comments.
Presumably the not-yet-finished reviewer's motivation for wanting others to wait for them is that they have some set of concerns that they feel other reviewers may not be examining. IMO, it'd be better to ask the reviewers that do have more time to take those concerns into account rather than blocking the commit. And of course - even after patches are committed, we still have release votes and the concept of release blocker issues as a final gate to allow people to express an opinion that particular code should not be released without changes. So the commit of a patch itself is not the only gate that exists before code is released. On Fri, Jan 25, 2019 at 3:43 AM Roman Leventov <leventov...@gmail.com> wrote: > The times that I suggested are IMO minimally reasonable for not provoking a > sense of rush and anxiety in people being poked. > > If we assume that people adhere to the guideline "do not comment on a pull > request unless you are willing to follow up on the edits" from here: > > https://github.com/druid-io/druid-io.github.io/blob/src/community/index.md#general-committer-guidelines > and don't forget about the PRs they started reviewing, poking appears to be > pointless. Poking and expecting the reviewer to respond "I didn't forget > about it, I'll continue my review soon" is not a good use of the time of > both people and IMO should be the common practice. > > Proposal reviews are good but the code should be reviewed thoroughly too. > Despite the proposal got enough approvals, the PR with the actual code > shouldn't be rushed into the codebase. > > On Fri, 25 Jan 2019 at 04:05, Gian Merlino <g...@apache.org> wrote: > > > The timelines you outlined seem quite slow. Especially "if there are > enough > > approvals, a PR could be merged not sooner than in two weeks since they > > left the last review comment". IMO, rather than delaying patches by so > > long, a better way to be courteous of a reviewer being too busy to review > > in a timely manner is to seek review from some other committer that has > > more time. > > > > In an extreme case, where the patch has issues that a reviewer feels must > > be addressed before the patch is merged, but the reviewer does not have > > time to hold up their end of that conversation, they can veto ( > > https://www.apache.org/foundation/voting.html#Veto), accompanied by a > > justification. This is a pretty blunt tool and should not be used much. > But > > it is there. > > > > I'm still optimistic that the discussion we've been having around > proposals > > is a good way to address a desire for reviewers to have their say, in a > way > > that doesn't slow down the development process so much. By starting > > conversations about changes earlier, it is a way for contributors to come > > together and agree on the general shape of changes before there is a > bunch > > of code to discuss. Hopefully that also makes code review more efficient > in > > terms of contributors' time, reviewers' time, and amount of calendar days > > that PRs are open for. > > > > On Thu, Jan 24, 2019 at 3:41 AM Roman Leventov <leven...@apache.org> > > wrote: > > > > > To foster calmness, respect, and consideration of people's busy > > schedules I > > > suggest the following etiquette: > > > > > > - When someone showed up in a PR and left some review comments, but > > didn't > > > explicitly approved the PR, poke them with comments like "@username do > > you > > > have more comments?" not sooner than in one week since they left the > last > > > review comment. > > > - Poke people no more frequently than once per week. > > > - If there are enough approvals, a PR could be merged not sooner than > in > > > two weeks since they left the last review comment. > > > - If someone created a valuable PR but then stopped responding to > review > > > comments and there are conflicts or tests/CI don't pass, a PR could be > > > recreated by another person not sooner than in three weeks since the > > > author's last activity in the PR. Their authorship should be preserved > by > > > cherry-picking their commits, squashing them locally, rebasing on top > of > > > current upstream master, creating a new PR and choosing "Rebase and > > merge" > > > option when merging the new PR instead of the default "Squash and > merge". > > > > > >
