Hi!
I have seen that recently many pull requests designate reviews by writing "@personA review please" or so. I am personally quite strongly against that, I think it hurts the community work: - The same few people get usually "designated" and will typically get overloaded and often not do the review. - At the same time, this discourages other community members from looking at the pull request, which is totally undesirable. - In general, review participation should be "pull based" (person decides what they want to work on) not "push based" (random person pushes work to another person). Push-based just creates the wrong feeling in a community of volunteers. - In many cases the designated reviews are not the ones most knowledgeable in the code, which is understandable, because how should contributors know whom to tag? Long story short, why don't we just drop that habit? Greetings, Stephan |
Thanks for bringing this up Stephan.
I completely agree with you. Cheers, Fabian 2017-01-16 12:42 GMT+01:00 Stephan Ewen <[hidden email]>: > Hi! > > I have seen that recently many pull requests designate reviews by writing > "@personA review please" or so. > > I am personally quite strongly against that, I think it hurts the community > work: > > - The same few people get usually "designated" and will typically get > overloaded and often not do the review. > > - At the same time, this discourages other community members from looking > at the pull request, which is totally undesirable. > > - In general, review participation should be "pull based" (person decides > what they want to work on) not "push based" (random person pushes work to > another person). Push-based just creates the wrong feeling in a community > of volunteers. > > - In many cases the designated reviews are not the ones most > knowledgeable in the code, which is understandable, because how should > contributors know whom to tag? > > > Long story short, why don't we just drop that habit? > > > Greetings, > Stephan > |
I also agree with all the points, especially when it comes to new PRs.
Though, when someone has started reviewing a PR and shows interest it probably makes sense to finish doing so. Wouldn’t tagging be acceptable there? In those case tagging triggers direct notifications, so that people already involved in a conversation get reminded and answer pending questions. > On 16 Jan 2017, at 12:45, Fabian Hueske <[hidden email]> wrote: > > Thanks for bringing this up Stephan. > I completely agree with you. > > Cheers, Fabian > > 2017-01-16 12:42 GMT+01:00 Stephan Ewen <[hidden email]>: > >> Hi! >> >> I have seen that recently many pull requests designate reviews by writing >> "@personA review please" or so. >> >> I am personally quite strongly against that, I think it hurts the community >> work: >> >> - The same few people get usually "designated" and will typically get >> overloaded and often not do the review. >> >> - At the same time, this discourages other community members from looking >> at the pull request, which is totally undesirable. >> >> - In general, review participation should be "pull based" (person decides >> what they want to work on) not "push based" (random person pushes work to >> another person). Push-based just creates the wrong feeling in a community >> of volunteers. >> >> - In many cases the designated reviews are not the ones most >> knowledgeable in the code, which is understandable, because how should >> contributors know whom to tag? >> >> >> Long story short, why don't we just drop that habit? >> >> >> Greetings, >> Stephan >> |
On 16 January 2017 at 12:59:04, Paris Carbone ([hidden email]) wrote:
> > Though, when someone has started reviewing a PR and shows interest > it probably makes sense to finish doing so. Wouldn’t tagging > be acceptable there? > In those case tagging triggers direct notifications, so that > people already involved in a conversation get reminded and answer > pending questions. I think that's totally fine Paris since it is more of a reminder in that case. Stephan is referring to PRs that have a last line in the description like "@XZY for review please". – Ufuk |
Hi all
View from my prospective: in middle of summer - 150 PR in middle of autumn - 180 now 206. This is mix of bugfixes and improvements. I understand that work on new features important, but when small and trivial fixes stay in states of PR more then 2-3 month, then all users think about changing engine on other product. Only way push people to merge this fixes in master it's tags. I don't speak about big changes, only about small and trivial with review less then 5 min. Features important, but if this features work incorrect, then user can select more stability product without any hesitation. Thanks Alexey Diomin 2017-01-16 16:36 GMT+04:00 Ufuk Celebi <[hidden email]>: > On 16 January 2017 at 12:59:04, Paris Carbone ([hidden email]) wrote: > > > Though, when someone has started reviewing a PR and shows interest > > it probably makes sense to finish doing so. Wouldn’t tagging > > be acceptable there? > > In those case tagging triggers direct notifications, so that > > people already involved in a conversation get reminded and answer > > pending questions. > > I think that's totally fine Paris since it is more of a reminder in that > case. > > Stephan is referring to PRs that have a last line in the description like > "@XZY for review please". > > – Ufuk > > > |
In reply to this post by Ufuk Celebi-2
Thanks for the comments.
@Paris - Ufuk has it right, tagging as a reminder (or just because it helps with referring to the comment from a specific reviewer) makes total sense to me, I would keep doing that. On Mon, Jan 16, 2017 at 1:36 PM, Ufuk Celebi <[hidden email]> wrote: > On 16 January 2017 at 12:59:04, Paris Carbone ([hidden email]) wrote: > > > Though, when someone has started reviewing a PR and shows interest > > it probably makes sense to finish doing so. Wouldn’t tagging > > be acceptable there? > > In those case tagging triggers direct notifications, so that > > people already involved in a conversation get reminded and answer > > pending questions. > > I think that's totally fine Paris since it is more of a reminder in that > case. > > Stephan is referring to PRs that have a last line in the description like > "@XZY for review please". > > – Ufuk > > > |
In reply to this post by Alexey Demin
Hi, Alexey
I will check abandoned PRs to reduce obviously outdated ones and add them to a cleanup list https://issues.apache.org/jira/browse/FLINK-5384 -----Original Message----- From: Alexey Demin [mailto:[hidden email]] Sent: Monday, January 16, 2017 5:05 PM To: [hidden email] Subject: Re: [DISCUSS] (Not) tagging reviewers Hi all View from my prospective: in middle of summer - 150 PR in middle of autumn - 180 now 206. This is mix of bugfixes and improvements. I understand that work on new features important, but when small and trivial fixes stay in states of PR more then 2-3 month, then all users think about changing engine on other product. Only way push people to merge this fixes in master it's tags. I don't speak about big changes, only about small and trivial with review less then 5 min. Features important, but if this features work incorrect, then user can select more stability product without any hesitation. Thanks Alexey Diomin 2017-01-16 16:36 GMT+04:00 Ufuk Celebi <[hidden email]>: > On 16 January 2017 at 12:59:04, Paris Carbone ([hidden email]) wrote: > > > Though, when someone has started reviewing a PR and shows interest > > it probably makes sense to finish doing so. Wouldn’t tagging be > > acceptable there? > > In those case tagging triggers direct notifications, so that people > > already involved in a conversation get reminded and answer pending > > questions. > > I think that's totally fine Paris since it is more of a reminder in > that case. > > Stephan is referring to PRs that have a last line in the description > like "@XZY for review please". > > – Ufuk > > > |
@Alexey - Pull Requests backlog is going pretty crazy, I agree.
That is not because the committers are not working on pull requests, there is simply so many of them. We are looking for new committers (and discussing in the PMC). Tagging is not going to make this better, I believe. It may make it worse, because it discourages non-tagged community members from picking up a pull request. On Mon, Jan 16, 2017 at 4:54 PM, Anton Solovev <[hidden email]> wrote: > Hi, Alexey > > I will check abandoned PRs to reduce obviously outdated ones and add them > to a cleanup list https://issues.apache.org/jira/browse/FLINK-5384 > > > -----Original Message----- > From: Alexey Demin [mailto:[hidden email]] > Sent: Monday, January 16, 2017 5:05 PM > To: [hidden email] > Subject: Re: [DISCUSS] (Not) tagging reviewers > > Hi all > > View from my prospective: > in middle of summer - 150 PR > in middle of autumn - 180 > now 206. > > This is mix of bugfixes and improvements. > I understand that work on new features important, but when small and > trivial fixes stay in states of PR more then 2-3 month, then all users > think about changing engine on other product. > > Only way push people to merge this fixes in master it's tags. > > I don't speak about big changes, only about small and trivial with review > less then 5 min. > > Features important, but if this features work incorrect, then user can > select more stability product without any hesitation. > > Thanks > Alexey Diomin > > > > 2017-01-16 16:36 GMT+04:00 Ufuk Celebi <[hidden email]>: > > > On 16 January 2017 at 12:59:04, Paris Carbone ([hidden email]) wrote: > > > > Though, when someone has started reviewing a PR and shows interest > > > it probably makes sense to finish doing so. Wouldn’t tagging be > > > acceptable there? > > > In those case tagging triggers direct notifications, so that people > > > already involved in a conversation get reminded and answer pending > > > questions. > > > > I think that's totally fine Paris since it is more of a reminder in > > that case. > > > > Stephan is referring to PRs that have a last line in the description > > like "@XZY for review please". > > > > – Ufuk > > > > > > > |
Hi,
Usually this problem can be well addressed by growing the community. We Just wondering whether there is a wiki to describe how non-committers can help on reviewing the patches? I'm glad to help out on reviewing patches. Regards, Haohui On Fri, Jan 20, 2017 at 7:42 AM Stephan Ewen <[hidden email]> wrote: > @Alexey - Pull Requests backlog is going pretty crazy, I agree. > > That is not because the committers are not working on pull requests, there > is simply so many of them. > We are looking for new committers (and discussing in the PMC). > > Tagging is not going to make this better, I believe. It may make it worse, > because it discourages non-tagged community members from picking up a pull > request. > > > > On Mon, Jan 16, 2017 at 4:54 PM, Anton Solovev <[hidden email]> > wrote: > > > Hi, Alexey > > > > I will check abandoned PRs to reduce obviously outdated ones and add them > > to a cleanup list https://issues.apache.org/jira/browse/FLINK-5384 > > > > > > -----Original Message----- > > From: Alexey Demin [mailto:[hidden email]] > > Sent: Monday, January 16, 2017 5:05 PM > > To: [hidden email] > > Subject: Re: [DISCUSS] (Not) tagging reviewers > > > > Hi all > > > > View from my prospective: > > in middle of summer - 150 PR > > in middle of autumn - 180 > > now 206. > > > > This is mix of bugfixes and improvements. > > I understand that work on new features important, but when small and > > trivial fixes stay in states of PR more then 2-3 month, then all users > > think about changing engine on other product. > > > > Only way push people to merge this fixes in master it's tags. > > > > I don't speak about big changes, only about small and trivial with review > > less then 5 min. > > > > Features important, but if this features work incorrect, then user can > > select more stability product without any hesitation. > > > > Thanks > > Alexey Diomin > > > > > > > > 2017-01-16 16:36 GMT+04:00 Ufuk Celebi <[hidden email]>: > > > > > On 16 January 2017 at 12:59:04, Paris Carbone ([hidden email]) wrote: > > > > > Though, when someone has started reviewing a PR and shows interest > > > > it probably makes sense to finish doing so. Wouldn’t tagging be > > > > acceptable there? > > > > In those case tagging triggers direct notifications, so that people > > > > already involved in a conversation get reminded and answer pending > > > > questions. > > > > > > I think that's totally fine Paris since it is more of a reminder in > > > that case. > > > > > > Stephan is referring to PRs that have a last line in the description > > > like "@XZY for review please". > > > > > > – Ufuk > > > > > > > > > > > > |
In reply to this post by Stephan Ewen
I totally agree with all of your ideas.
Best wishes, SunJincheng. Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > Hi! > > > > I have seen that recently many pull requests designate reviews by writing > > "@personA review please" or so. > > > > I am personally quite strongly against that, I think it hurts the community > > work: > > > > - The same few people get usually "designated" and will typically get > > overloaded and often not do the review. > > > > - At the same time, this discourages other community members from looking > > at the pull request, which is totally undesirable. > > > > - In general, review participation should be "pull based" (person decides > > what they want to work on) not "push based" (random person pushes work to > > another person). Push-based just creates the wrong feeling in a community > > of volunteers. > > > > - In many cases the designated reviews are not the ones most > > knowledgeable in the code, which is understandable, because how should > > contributors know whom to tag? > > > > > > Long story short, why don't we just drop that habit? > > > > > > Greetings, > > Stephan > > |
Hi Haohui,
reviewing pull requests is a great way of contributing to the community! I am not aware of specific instructions for the review process. The are some dos and don'ts on our "contribute code" page [1] that should be considered. Apart from that, I think the best way to start is to become familiar with a certain part of the code base (reading code, contributing) and then to look out for pull requests that address the part you are familiar with. The review does not have to cover all aspects of a PR (a committer will have a look as well), but from my personal experience the effort to review a PR is often much lower if some other person has had a look at it already and gave feedback. I think this can help a lot to reduce the review "load" on the committers. Maybe you find some contributors who are interested in the same components as you and you can start reviewing each others code. Thanks, Fabian [1] http://flink.apache.org/contribute-code.html#coding-guidelines 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email]>: > I totally agree with all of your ideas. > > > > > > > > > > > Best wishes, > > > > SunJincheng. > > Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > > > Hi! > > > > > > > > I have seen that recently many pull requests designate reviews by writing > > > > "@personA review please" or so. > > > > > > > > I am personally quite strongly against that, I think it hurts the > community > > > > work: > > > > > > > > - The same few people get usually "designated" and will typically get > > > > overloaded and often not do the review. > > > > > > > > - At the same time, this discourages other community members from > looking > > > > at the pull request, which is totally undesirable. > > > > > > > > - In general, review participation should be "pull based" (person > decides > > > > what they want to work on) not "push based" (random person pushes work to > > > > another person). Push-based just creates the wrong feeling in a community > > > > of volunteers. > > > > > > > > - In many cases the designated reviews are not the ones most > > > > knowledgeable in the code, which is understandable, because how should > > > > contributors know whom to tag? > > > > > > > > > > > > Long story short, why don't we just drop that habit? > > > > > > > > > > > > Greetings, > > > > Stephan > > > > > |
It seems I'm in a bit of a minority here but I like the @R tags. There are
simply to many pull request for someone to keep track of all of them and if someone things that a certain person would be good for reviewing a change then tagging them helps them notice the PR. I think the tag should not mean that only that person can/should review the PR, it should serve as a proposal. I'm happy to not use it anymore if everyone else doesn't like them. On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <[hidden email]> wrote: > Hi Haohui, > > reviewing pull requests is a great way of contributing to the community! > > I am not aware of specific instructions for the review process. The are > some dos and don'ts on our "contribute code" page [1] that should be > considered. Apart from that, I think the best way to start is to become > familiar with a certain part of the code base (reading code, contributing) > and then to look out for pull requests that address the part you are > familiar with. > > The review does not have to cover all aspects of a PR (a committer will > have a look as well), but from my personal experience the effort to review > a PR is often much lower if some other person has had a look at it already > and gave feedback. > I think this can help a lot to reduce the review "load" on the committers. > Maybe you find some contributors who are interested in the same components > as you and you can start reviewing each others code. > > Thanks, > Fabian > > [1] http://flink.apache.org/contribute-code.html#coding-guidelines > > > 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email]>: > > > I totally agree with all of your ideas. > > > > > > > > > > > > > > > > > > > > > > Best wishes, > > > > > > > > SunJincheng. > > > > Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > > > > > Hi! > > > > > > > > > > > > I have seen that recently many pull requests designate reviews by > writing > > > > > > "@personA review please" or so. > > > > > > > > > > > > I am personally quite strongly against that, I think it hurts the > > community > > > > > > work: > > > > > > > > > > > > - The same few people get usually "designated" and will typically get > > > > > > overloaded and often not do the review. > > > > > > > > > > > > - At the same time, this discourages other community members from > > looking > > > > > > at the pull request, which is totally undesirable. > > > > > > > > > > > > - In general, review participation should be "pull based" (person > > decides > > > > > > what they want to work on) not "push based" (random person pushes work > to > > > > > > another person). Push-based just creates the wrong feeling in a > community > > > > > > of volunteers. > > > > > > > > > > > > - In many cases the designated reviews are not the ones most > > > > > > knowledgeable in the code, which is understandable, because how should > > > > > > contributors know whom to tag? > > > > > > > > > > > > > > > > > > Long story short, why don't we just drop that habit? > > > > > > > > > > > > > > > > > > Greetings, > > > > > > Stephan > > > > > > > > > |
I was wondering how this relates to the shepherding of PRs we have
discussed in the past. If I make a PR for an issue reported from a specific committer, doesn't tagging them make sense? Has the shepherding of PRs been tried out? On Tue, Jan 24, 2017 at 12:17 PM, Aljoscha Krettek <[hidden email]> wrote: > It seems I'm in a bit of a minority here but I like the @R tags. There are > simply to many pull request for someone to keep track of all of them and if > someone things that a certain person would be good for reviewing a change > then tagging them helps them notice the PR. > > I think the tag should not mean that only that person can/should review the > PR, it should serve as a proposal. > > I'm happy to not use it anymore if everyone else doesn't like them. > > On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <[hidden email]> wrote: > > > Hi Haohui, > > > > reviewing pull requests is a great way of contributing to the community! > > > > I am not aware of specific instructions for the review process. The are > > some dos and don'ts on our "contribute code" page [1] that should be > > considered. Apart from that, I think the best way to start is to become > > familiar with a certain part of the code base (reading code, > contributing) > > and then to look out for pull requests that address the part you are > > familiar with. > > > > The review does not have to cover all aspects of a PR (a committer will > > have a look as well), but from my personal experience the effort to > review > > a PR is often much lower if some other person has had a look at it > already > > and gave feedback. > > I think this can help a lot to reduce the review "load" on the > committers. > > Maybe you find some contributors who are interested in the same > components > > as you and you can start reviewing each others code. > > > > Thanks, > > Fabian > > > > [1] http://flink.apache.org/contribute-code.html#coding-guidelines > > > > > > 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email]>: > > > > > I totally agree with all of your ideas. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best wishes, > > > > > > > > > > > > SunJincheng. > > > > > > Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > > > > > > > Hi! > > > > > > > > > > > > > > > > I have seen that recently many pull requests designate reviews by > > writing > > > > > > > > "@personA review please" or so. > > > > > > > > > > > > > > > > I am personally quite strongly against that, I think it hurts the > > > community > > > > > > > > work: > > > > > > > > > > > > > > > > - The same few people get usually "designated" and will typically > get > > > > > > > > overloaded and often not do the review. > > > > > > > > > > > > > > > > - At the same time, this discourages other community members from > > > looking > > > > > > > > at the pull request, which is totally undesirable. > > > > > > > > > > > > > > > > - In general, review participation should be "pull based" (person > > > decides > > > > > > > > what they want to work on) not "push based" (random person pushes > work > > to > > > > > > > > another person). Push-based just creates the wrong feeling in a > > community > > > > > > > > of volunteers. > > > > > > > > > > > > > > > > - In many cases the designated reviews are not the ones most > > > > > > > > knowledgeable in the code, which is understandable, because how > should > > > > > > > > contributors know whom to tag? > > > > > > > > > > > > > > > > > > > > > > > > Long story short, why don't we just drop that habit? > > > > > > > > > > > > > > > > > > > > > > > > Greetings, > > > > > > > > Stephan > > > > > > > > > > > > > > |
I agree with Aljoscha that tagging the PRs helps me to get notified about
PRs where I could be of help. But I also think that it should not be the ultimate responsibility of the tagged person(s) to review the code. Everyone should feel empowered to do so. And also the tagging does not free oneself from browsing the PR list to check out the open PRs. @Theo, usually this notification should already happen via the JIRA integration given that the person who created the JIRA issue also watches it. Cheers, Till On Tue, Jan 24, 2017 at 1:29 PM, Theodore Vasiloudis < [hidden email]> wrote: > I was wondering how this relates to the shepherding of PRs we have > discussed in the past. If I make a PR for an issue reported from a specific > committer, doesn't tagging them make sense? > > Has the shepherding of PRs been tried out? > > On Tue, Jan 24, 2017 at 12:17 PM, Aljoscha Krettek <[hidden email]> > wrote: > > > It seems I'm in a bit of a minority here but I like the @R tags. There > are > > simply to many pull request for someone to keep track of all of them and > if > > someone things that a certain person would be good for reviewing a change > > then tagging them helps them notice the PR. > > > > I think the tag should not mean that only that person can/should review > the > > PR, it should serve as a proposal. > > > > I'm happy to not use it anymore if everyone else doesn't like them. > > > > On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <[hidden email]> wrote: > > > > > Hi Haohui, > > > > > > reviewing pull requests is a great way of contributing to the > community! > > > > > > I am not aware of specific instructions for the review process. The are > > > some dos and don'ts on our "contribute code" page [1] that should be > > > considered. Apart from that, I think the best way to start is to become > > > familiar with a certain part of the code base (reading code, > > contributing) > > > and then to look out for pull requests that address the part you are > > > familiar with. > > > > > > The review does not have to cover all aspects of a PR (a committer will > > > have a look as well), but from my personal experience the effort to > > review > > > a PR is often much lower if some other person has had a look at it > > already > > > and gave feedback. > > > I think this can help a lot to reduce the review "load" on the > > committers. > > > Maybe you find some contributors who are interested in the same > > components > > > as you and you can start reviewing each others code. > > > > > > Thanks, > > > Fabian > > > > > > [1] http://flink.apache.org/contribute-code.html#coding-guidelines > > > > > > > > > 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email]>: > > > > > > > I totally agree with all of your ideas. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best wishes, > > > > > > > > > > > > > > > > SunJincheng. > > > > > > > > Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > I have seen that recently many pull requests designate reviews by > > > writing > > > > > > > > > > "@personA review please" or so. > > > > > > > > > > > > > > > > > > > > I am personally quite strongly against that, I think it hurts the > > > > community > > > > > > > > > > work: > > > > > > > > > > > > > > > > > > > > - The same few people get usually "designated" and will typically > > get > > > > > > > > > > overloaded and often not do the review. > > > > > > > > > > > > > > > > > > > > - At the same time, this discourages other community members from > > > > looking > > > > > > > > > > at the pull request, which is totally undesirable. > > > > > > > > > > > > > > > > > > > > - In general, review participation should be "pull based" (person > > > > decides > > > > > > > > > > what they want to work on) not "push based" (random person pushes > > work > > > to > > > > > > > > > > another person). Push-based just creates the wrong feeling in a > > > community > > > > > > > > > > of volunteers. > > > > > > > > > > > > > > > > > > > > - In many cases the designated reviews are not the ones most > > > > > > > > > > knowledgeable in the code, which is understandable, because how > > should > > > > > > > > > > contributors know whom to tag? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Long story short, why don't we just drop that habit? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Greetings, > > > > > > > > > > Stephan > > > > > > > > > > > > > > > > > > > > |
+1. Nice to be pinged if required, but relying on specific people to review
simply does not scale. Popping up one level, occasionally the fact that people tag other people is because the PRs are left unreviewed for a while (or they believe the PRs will not be reviewed unless they explicitly ping other folks). Having more people (not necessarily committers) to participate the review process will go a long way. I took a quick skim on the PRs and I noticed that only a few of them are actually in mergeable shapes (i.e., properly rebased and passing CI). Maybe it is a good idea to proactively clean up the open pull requests to make it easier for people to check them out? That way there might be more help for reviews from the community side. Regards, Haohui On Tue, Jan 24, 2017 at 5:07 AM Till Rohrmann <[hidden email]> wrote: > I agree with Aljoscha that tagging the PRs helps me to get notified about > PRs where I could be of help. But I also think that it should not be the > ultimate responsibility of the tagged person(s) to review the code. > Everyone should feel empowered to do so. And also the tagging does not free > oneself from browsing the PR list to check out the open PRs. > > @Theo, usually this notification should already happen via the JIRA > integration given that the person who created the JIRA issue also watches > it. > > Cheers, > Till > > On Tue, Jan 24, 2017 at 1:29 PM, Theodore Vasiloudis < > [hidden email]> wrote: > > > I was wondering how this relates to the shepherding of PRs we have > > discussed in the past. If I make a PR for an issue reported from a > specific > > committer, doesn't tagging them make sense? > > > > Has the shepherding of PRs been tried out? > > > > On Tue, Jan 24, 2017 at 12:17 PM, Aljoscha Krettek <[hidden email]> > > wrote: > > > > > It seems I'm in a bit of a minority here but I like the @R tags. There > > are > > > simply to many pull request for someone to keep track of all of them > and > > if > > > someone things that a certain person would be good for reviewing a > change > > > then tagging them helps them notice the PR. > > > > > > I think the tag should not mean that only that person can/should review > > the > > > PR, it should serve as a proposal. > > > > > > I'm happy to not use it anymore if everyone else doesn't like them. > > > > > > On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <[hidden email]> wrote: > > > > > > > Hi Haohui, > > > > > > > > reviewing pull requests is a great way of contributing to the > > community! > > > > > > > > I am not aware of specific instructions for the review process. The > are > > > > some dos and don'ts on our "contribute code" page [1] that should be > > > > considered. Apart from that, I think the best way to start is to > become > > > > familiar with a certain part of the code base (reading code, > > > contributing) > > > > and then to look out for pull requests that address the part you are > > > > familiar with. > > > > > > > > The review does not have to cover all aspects of a PR (a committer > will > > > > have a look as well), but from my personal experience the effort to > > > review > > > > a PR is often much lower if some other person has had a look at it > > > already > > > > and gave feedback. > > > > I think this can help a lot to reduce the review "load" on the > > > committers. > > > > Maybe you find some contributors who are interested in the same > > > components > > > > as you and you can start reviewing each others code. > > > > > > > > Thanks, > > > > Fabian > > > > > > > > [1] http://flink.apache.org/contribute-code.html#coding-guidelines > > > > > > > > > > > > 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email]>: > > > > > > > > > I totally agree with all of your ideas. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best wishes, > > > > > > > > > > > > > > > > > > > > SunJincheng. > > > > > > > > > > Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > > > > > I have seen that recently many pull requests designate reviews by > > > > writing > > > > > > > > > > > > "@personA review please" or so. > > > > > > > > > > > > > > > > > > > > > > > > I am personally quite strongly against that, I think it hurts the > > > > > community > > > > > > > > > > > > work: > > > > > > > > > > > > > > > > > > > > > > > > - The same few people get usually "designated" and will > typically > > > get > > > > > > > > > > > > overloaded and often not do the review. > > > > > > > > > > > > > > > > > > > > > > > > - At the same time, this discourages other community members > from > > > > > looking > > > > > > > > > > > > at the pull request, which is totally undesirable. > > > > > > > > > > > > > > > > > > > > > > > > - In general, review participation should be "pull based" > (person > > > > > decides > > > > > > > > > > > > what they want to work on) not "push based" (random person pushes > > > work > > > > to > > > > > > > > > > > > another person). Push-based just creates the wrong feeling in a > > > > community > > > > > > > > > > > > of volunteers. > > > > > > > > > > > > > > > > > > > > > > > > - In many cases the designated reviews are not the ones most > > > > > > > > > > > > knowledgeable in the code, which is understandable, because how > > > should > > > > > > > > > > > > contributors know whom to tag? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Long story short, why don't we just drop that habit? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Greetings, > > > > > > > > > > > > Stephan > > > > > > > > > > > > > > > > > > > > > > > > > > > |
Hi, all
Thanks to the reviewers’ hard working. When a new issue or pull request, the issue provider hope can be reviewed quickly. So we usually “@somebody”, it’s only a request for quick review. Maybe we can identify whether this issue is indeed necessary or its obvious problem, so as to decide whether close it or review it forward. I think we can have one reviewer whose main responsibility is identify the unworthiness issue and PR and close them. :) Best, Jinkui Shi > On Jan 26, 2017, at 13:58, Haohui Mai <[hidden email]> wrote: > > +1. Nice to be pinged if required, but relying on specific people to review > simply does not scale. > > Popping up one level, occasionally the fact that people tag other people is > because the PRs are left unreviewed for a while (or they believe the PRs > will not be reviewed unless they explicitly ping other folks). Having more > people (not necessarily committers) to participate the review process will > go a long way. > > I took a quick skim on the PRs and I noticed that only a few of them are > actually in mergeable shapes (i.e., properly rebased and passing CI). > > Maybe it is a good idea to proactively clean up the open pull requests to > make it easier for people to check them out? That way there might be more > help for reviews from the community side. > > Regards, > Haohui > > On Tue, Jan 24, 2017 at 5:07 AM Till Rohrmann <[hidden email]> wrote: > >> I agree with Aljoscha that tagging the PRs helps me to get notified about >> PRs where I could be of help. But I also think that it should not be the >> ultimate responsibility of the tagged person(s) to review the code. >> Everyone should feel empowered to do so. And also the tagging does not free >> oneself from browsing the PR list to check out the open PRs. >> >> @Theo, usually this notification should already happen via the JIRA >> integration given that the person who created the JIRA issue also watches >> it. >> >> Cheers, >> Till >> >> On Tue, Jan 24, 2017 at 1:29 PM, Theodore Vasiloudis < >> [hidden email]> wrote: >> >>> I was wondering how this relates to the shepherding of PRs we have >>> discussed in the past. If I make a PR for an issue reported from a >> specific >>> committer, doesn't tagging them make sense? >>> >>> Has the shepherding of PRs been tried out? >>> >>> On Tue, Jan 24, 2017 at 12:17 PM, Aljoscha Krettek <[hidden email]> >>> wrote: >>> >>>> It seems I'm in a bit of a minority here but I like the @R tags. There >>> are >>>> simply to many pull request for someone to keep track of all of them >> and >>> if >>>> someone things that a certain person would be good for reviewing a >> change >>>> then tagging them helps them notice the PR. >>>> >>>> I think the tag should not mean that only that person can/should review >>> the >>>> PR, it should serve as a proposal. >>>> >>>> I'm happy to not use it anymore if everyone else doesn't like them. >>>> >>>> On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <[hidden email]> wrote: >>>> >>>>> Hi Haohui, >>>>> >>>>> reviewing pull requests is a great way of contributing to the >>> community! >>>>> >>>>> I am not aware of specific instructions for the review process. The >> are >>>>> some dos and don'ts on our "contribute code" page [1] that should be >>>>> considered. Apart from that, I think the best way to start is to >> become >>>>> familiar with a certain part of the code base (reading code, >>>> contributing) >>>>> and then to look out for pull requests that address the part you are >>>>> familiar with. >>>>> >>>>> The review does not have to cover all aspects of a PR (a committer >> will >>>>> have a look as well), but from my personal experience the effort to >>>> review >>>>> a PR is often much lower if some other person has had a look at it >>>> already >>>>> and gave feedback. >>>>> I think this can help a lot to reduce the review "load" on the >>>> committers. >>>>> Maybe you find some contributors who are interested in the same >>>> components >>>>> as you and you can start reviewing each others code. >>>>> >>>>> Thanks, >>>>> Fabian >>>>> >>>>> [1] http://flink.apache.org/contribute-code.html#coding-guidelines >>>>> >>>>> >>>>> 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email]>: >>>>> >>>>>> I totally agree with all of your ideas. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Best wishes, >>>>>> >>>>>> >>>>>> >>>>>> SunJincheng. >>>>>> >>>>>> Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: >>>>>> >>>>>>> Hi! >>>>>>> >>>>>>> >>>>>>> >>>>>>> I have seen that recently many pull requests designate reviews by >>>>> writing >>>>>>> >>>>>>> "@personA review please" or so. >>>>>>> >>>>>>> >>>>>>> >>>>>>> I am personally quite strongly against that, I think it hurts the >>>>>> community >>>>>>> >>>>>>> work: >>>>>>> >>>>>>> >>>>>>> >>>>>>> - The same few people get usually "designated" and will >> typically >>>> get >>>>>>> >>>>>>> overloaded and often not do the review. >>>>>>> >>>>>>> >>>>>>> >>>>>>> - At the same time, this discourages other community members >> from >>>>>> looking >>>>>>> >>>>>>> at the pull request, which is totally undesirable. >>>>>>> >>>>>>> >>>>>>> >>>>>>> - In general, review participation should be "pull based" >> (person >>>>>> decides >>>>>>> >>>>>>> what they want to work on) not "push based" (random person pushes >>>> work >>>>> to >>>>>>> >>>>>>> another person). Push-based just creates the wrong feeling in a >>>>> community >>>>>>> >>>>>>> of volunteers. >>>>>>> >>>>>>> >>>>>>> >>>>>>> - In many cases the designated reviews are not the ones most >>>>>>> >>>>>>> knowledgeable in the code, which is understandable, because how >>>> should >>>>>>> >>>>>>> contributors know whom to tag? >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Long story short, why don't we just drop that habit? >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Greetings, >>>>>>> >>>>>>> Stephan >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> |
In reply to this post by Haohui Mai
> I took a quick skim on the PRs and I noticed that only a few of them are
actually in mergeable shapes (i.e., properly rebased and passing CI). Although TravisCI is quite unstable, Flink executes multiple tests with different configurations so you'll want to instead look at which tests are passing. On Thu, Jan 26, 2017 at 12:58 AM, Haohui Mai <[hidden email]> wrote: > +1. Nice to be pinged if required, but relying on specific people to review > simply does not scale. > > Popping up one level, occasionally the fact that people tag other people is > because the PRs are left unreviewed for a while (or they believe the PRs > will not be reviewed unless they explicitly ping other folks). Having more > people (not necessarily committers) to participate the review process will > go a long way. > > I took a quick skim on the PRs and I noticed that only a few of them are > actually in mergeable shapes (i.e., properly rebased and passing CI). > > Maybe it is a good idea to proactively clean up the open pull requests to > make it easier for people to check them out? That way there might be more > help for reviews from the community side. > > Regards, > Haohui > > On Tue, Jan 24, 2017 at 5:07 AM Till Rohrmann <[hidden email]> > wrote: > > > I agree with Aljoscha that tagging the PRs helps me to get notified about > > PRs where I could be of help. But I also think that it should not be the > > ultimate responsibility of the tagged person(s) to review the code. > > Everyone should feel empowered to do so. And also the tagging does not > free > > oneself from browsing the PR list to check out the open PRs. > > > > @Theo, usually this notification should already happen via the JIRA > > integration given that the person who created the JIRA issue also watches > > it. > > > > Cheers, > > Till > > > > On Tue, Jan 24, 2017 at 1:29 PM, Theodore Vasiloudis < > > [hidden email]> wrote: > > > > > I was wondering how this relates to the shepherding of PRs we have > > > discussed in the past. If I make a PR for an issue reported from a > > specific > > > committer, doesn't tagging them make sense? > > > > > > Has the shepherding of PRs been tried out? > > > > > > On Tue, Jan 24, 2017 at 12:17 PM, Aljoscha Krettek < > [hidden email]> > > > wrote: > > > > > > > It seems I'm in a bit of a minority here but I like the @R tags. > There > > > are > > > > simply to many pull request for someone to keep track of all of them > > and > > > if > > > > someone things that a certain person would be good for reviewing a > > change > > > > then tagging them helps them notice the PR. > > > > > > > > I think the tag should not mean that only that person can/should > review > > > the > > > > PR, it should serve as a proposal. > > > > > > > > I'm happy to not use it anymore if everyone else doesn't like them. > > > > > > > > On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <[hidden email]> > wrote: > > > > > > > > > Hi Haohui, > > > > > > > > > > reviewing pull requests is a great way of contributing to the > > > community! > > > > > > > > > > I am not aware of specific instructions for the review process. The > > are > > > > > some dos and don'ts on our "contribute code" page [1] that should > be > > > > > considered. Apart from that, I think the best way to start is to > > become > > > > > familiar with a certain part of the code base (reading code, > > > > contributing) > > > > > and then to look out for pull requests that address the part you > are > > > > > familiar with. > > > > > > > > > > The review does not have to cover all aspects of a PR (a committer > > will > > > > > have a look as well), but from my personal experience the effort to > > > > review > > > > > a PR is often much lower if some other person has had a look at it > > > > already > > > > > and gave feedback. > > > > > I think this can help a lot to reduce the review "load" on the > > > > committers. > > > > > Maybe you find some contributors who are interested in the same > > > > components > > > > > as you and you can start reviewing each others code. > > > > > > > > > > Thanks, > > > > > Fabian > > > > > > > > > > [1] http://flink.apache.org/contribute-code.html#coding-guidelines > > > > > > > > > > > > > > > 2017-01-20 23:02 GMT+01:00 jincheng sun <[hidden email] > >: > > > > > > > > > > > I totally agree with all of your ideas. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best wishes, > > > > > > > > > > > > > > > > > > > > > > > > SunJincheng. > > > > > > > > > > > > Stephan Ewen <[hidden email]>于2017年1月16日 周一19:42写道: > > > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have seen that recently many pull requests designate reviews > by > > > > > writing > > > > > > > > > > > > > > "@personA review please" or so. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I am personally quite strongly against that, I think it hurts > the > > > > > > community > > > > > > > > > > > > > > work: > > > > > > > > > > > > > > > > > > > > > > > > > > > > - The same few people get usually "designated" and will > > typically > > > > get > > > > > > > > > > > > > > overloaded and often not do the review. > > > > > > > > > > > > > > > > > > > > > > > > > > > > - At the same time, this discourages other community members > > from > > > > > > looking > > > > > > > > > > > > > > at the pull request, which is totally undesirable. > > > > > > > > > > > > > > > > > > > > > > > > > > > > - In general, review participation should be "pull based" > > (person > > > > > > decides > > > > > > > > > > > > > > what they want to work on) not "push based" (random person > pushes > > > > work > > > > > to > > > > > > > > > > > > > > another person). Push-based just creates the wrong feeling in a > > > > > community > > > > > > > > > > > > > > of volunteers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > - In many cases the designated reviews are not the ones most > > > > > > > > > > > > > > knowledgeable in the code, which is understandable, because how > > > > should > > > > > > > > > > > > > > contributors know whom to tag? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Long story short, why don't we just drop that habit? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Greetings, > > > > > > > > > > > > > > Stephan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > |
Free forum by Nabble | Edit this page |