Re: The etiquette of pocking people on Github and the policy when people stop responding
I think poking preferences are a fairly personal/subjective thing, and I
don't think we need a general formalized policy presently given the size of
our committer pool. I don't mind being poked often, for example.
I think it's enough to have an informal social understanding amongst the
committers for now ("this committer prefers to be poked less frequently,
let's keep that in mind").
For the case where a PR has enough approvals for merge but an incomplete
review from another committer, I think it's good to give that committer
some time to complete their review, but I would suggest a period of a 3-4
days; two weeks seems too long to me.
Thanks,
Jon
On Wed, Jan 30, 2019 at 5:22 AM Roman Leventov
wrote:
> On Tue, 29 Jan 2019 at 01:30, Fangjin Yang wrote:
>
> > I disagree with Roman's suggestions. If a PR has enough votes, we should
> > trust the committers approving the PR and move forward.
> >
>
> There is a specific committer who merges a PR. If this happens while it's
> not made clear that somebody who left comments before doesn't have any more
> comments, the whole situation looks to me more like disregard of that
> person's opinion. The trust to other committers doesn't help to make the
> situation look much better, IMO.
>
Re: The etiquette of pocking people on Github and the policy when people stop responding
On Tue, 29 Jan 2019 at 01:30, Fangjin Yang wrote: > I disagree with Roman's suggestions. If a PR has enough votes, we should > trust the committers approving the PR and move forward. > There is a specific committer who merges a PR. If this happens while it's not made clear that somebody who left comments before doesn't have any more comments, the whole situation looks to me more like disregard of that person's opinion. The trust to other committers doesn't help to make the situation look much better, IMO.
Re: The etiquette of pocking people on Github and the policy when people stop responding
On Tue, 29 Jan 2019 at 00:28, Gian Merlino wrote: > 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. > There is "Development Blocker" tag for such situations. What do you think if for PRs tagged "Development Blocker" the "poking period" is recommended to be 3 working days, and a week for other PRs?
Re: The etiquette of pocking people on Github and the policy when people stop responding
> 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 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 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 > > wrote: > > > > > On Fri, 25 Jan 2019 at 23:12, Gian Merlino 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. > > > > > >
Re: The etiquette of pocking people on Github and the policy when people stop responding
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 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 > wrote: > > > On Fri, 25 Jan 2019 at 23:12, Gian Merlino 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. > > >
Re: The etiquette of pocking people on Github and the policy when people stop responding
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 wrote: > On Fri, 25 Jan 2019 at 23:12, Gian Merlino 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. >
Re: The etiquette of pocking people on Github and the policy when people stop responding
On Fri, 25 Jan 2019 at 23:12, Gian Merlino 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.
Re: The etiquette of pocking people on Github and the policy when people stop responding
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 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 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 > > 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". > > > > > >
Re: The etiquette of pocking people on Github and the policy when people stop responding
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 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 > 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". > > >
Re: The etiquette of pocking people on Github and the policy when people stop responding
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 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". >
