Re: [DISCUSS] Review/merge phase, and post-review
In practice, I usually wait some more when the changes look complicated, when there are many reviews/discussions, when the change can potentially be controversial, etc. When I think its pretty clear to go, for example, multiple approvals from committers, when the changes look pretty clear and straightforward, etc. I just go ahead. I think the post review stuffs can happen. It needs some overhead to revert or add some more changes but I think its fine, for example, unless we consistnetly/frequently find non trivial issues. I personally just leave it and trust this call from individual committers who merge. On Sat, 14 Nov 2020, 16:54 Mridul Muralidharan, wrote: > > I try to follow the second option. > In general, when multiple reviewers are looking at the code, sometimes > addressing review comments might open up other avenues of > discussion/optimization/design discussions : atleast in core, I have seen > this happen often. > > A day or so delay is worth the increased scrutiny and better > design/reduced bugs. > > Regards, > Mridul > > On Sat, Nov 14, 2020 at 1:47 AM Jungtaek Lim > wrote: > >> I see some voices that it's not sufficient to understand the topic. Let >> me elaborate this a bit more. >> >> 1. There're multiple reviewers reviewing the PR. (Say, A, B, C, D) >> 2. A and B leaves review comments on the PR, but no one makes the >> explicit indication that these review comments are the final one. >> 3. The author of the PR addresses the review comments. >> 4. C checks that the review comments from A and B are addressed, and >> merges the PR. In parallel (or a bit later), A is trying to check whether >> the review comments are addressed (or even more, A could provide more >> review comments afterwards), and realized the PR is already merged. >> >> Saying again, there's "technically" no incorrect point. Let's give >> another example of what I said "trade-off". >> >> 1. There're multiple reviewers reviewing the PR. (Say, A, B, C, D) >> 2. A and B leaves review comments on the PR, but no one makes the >> explicit indication that these review comments are the final one. >> 3. The author of the PR addresses the review comments. >> 4. C checks that the review comments from A and B are addressed, and asks >> A and B to confirm whether there's no further review comments, with the >> condition that it will be merged in a few days later if there's no further >> feedback. >> 5. If A and B confirms or A and B doesn't provide new feedback in the >> period, C merges the PR. If A or B provides new feedback, go back to 3 with >> resetting the days. >> >> This is what we tend to comment as "@A @B I'll leave this a few days more >> to see if anyone has further comments. Otherwise I'll merge this.". >> >> I see both are used across various PRs, so it's not really something I >> want to blame. Just want to make us think about what would be the ideal >> approach we'd be better to prefer. >> >> >> On Sat, Nov 14, 2020 at 3:46 PM Jungtaek Lim < >> kabhwan.opensou...@gmail.com> wrote: >> >>> Oh sorry that was gone with flame (please just consider it as my fault) >>> and I just removed all comments. >>> >>> Btw, when I always initiate discussions, I really do love to start >>> discussion "without" specific instances which tend to go blaming each >>> other. I understand it's not easy to discuss without taking examples, but >>> I'll try to explain the situation on my best instead. Please let me know if >>> there's some ambiguous or unclear thing to think about. >>> >>> On Sat, Nov 14, 2020 at 3:41 PM Sean Owen wrote: >>> I am sure you are referring to some specific instances but I have not followed enough to know what they are. Can you point them out? I think that is most productive for everyone to understand. On Fri, Nov 13, 2020 at 10:16 PM Jungtaek Lim < kabhwan.opensou...@gmail.com> wrote: > Hi devs, > > I know this is a super sensitive topic and at a risk of flame, but > just like to try this. My apologies first. > Assuming we all know about the ASF policy about code commit and I > don't see Spark project has any explicit BYLAWS, it's technically possible > to do anything for committers to do during merging. > > Sometimes this goes a bit depressing for reviewers, regardless of the > intention, when merger makes a judgement by oneself to merge while the > reviewers are still in the review phase. I observed the practice is used > frequently, under the fact that we have post-review to address further > comments later. > > I know about the concern that it's sometimes blocking unintentionally > if we require merger to gather consensus about the merge from reviewers, > but we also have some other practice holding on merging for a couple of > days and noticing to reviewers whether they have further comments or not, > which is I think a good trade-off. > > Exclude the cases where we're in release blocker
Re: [DISCUSS] Review/merge phase, and post-review
I try to follow the second option. In general, when multiple reviewers are looking at the code, sometimes addressing review comments might open up other avenues of discussion/optimization/design discussions : atleast in core, I have seen this happen often. A day or so delay is worth the increased scrutiny and better design/reduced bugs. Regards, Mridul On Sat, Nov 14, 2020 at 1:47 AM Jungtaek Lim wrote: > I see some voices that it's not sufficient to understand the topic. Let me > elaborate this a bit more. > > 1. There're multiple reviewers reviewing the PR. (Say, A, B, C, D) > 2. A and B leaves review comments on the PR, but no one makes the explicit > indication that these review comments are the final one. > 3. The author of the PR addresses the review comments. > 4. C checks that the review comments from A and B are addressed, and > merges the PR. In parallel (or a bit later), A is trying to check whether > the review comments are addressed (or even more, A could provide more > review comments afterwards), and realized the PR is already merged. > > Saying again, there's "technically" no incorrect point. Let's give another > example of what I said "trade-off". > > 1. There're multiple reviewers reviewing the PR. (Say, A, B, C, D) > 2. A and B leaves review comments on the PR, but no one makes the explicit > indication that these review comments are the final one. > 3. The author of the PR addresses the review comments. > 4. C checks that the review comments from A and B are addressed, and asks > A and B to confirm whether there's no further review comments, with the > condition that it will be merged in a few days later if there's no further > feedback. > 5. If A and B confirms or A and B doesn't provide new feedback in the > period, C merges the PR. If A or B provides new feedback, go back to 3 with > resetting the days. > > This is what we tend to comment as "@A @B I'll leave this a few days more > to see if anyone has further comments. Otherwise I'll merge this.". > > I see both are used across various PRs, so it's not really something I > want to blame. Just want to make us think about what would be the ideal > approach we'd be better to prefer. > > > On Sat, Nov 14, 2020 at 3:46 PM Jungtaek Lim > wrote: > >> Oh sorry that was gone with flame (please just consider it as my fault) >> and I just removed all comments. >> >> Btw, when I always initiate discussions, I really do love to start >> discussion "without" specific instances which tend to go blaming each >> other. I understand it's not easy to discuss without taking examples, but >> I'll try to explain the situation on my best instead. Please let me know if >> there's some ambiguous or unclear thing to think about. >> >> On Sat, Nov 14, 2020 at 3:41 PM Sean Owen wrote: >> >>> I am sure you are referring to some specific instances but I have not >>> followed enough to know what they are. Can you point them out? I think that >>> is most productive for everyone to understand. >>> >>> On Fri, Nov 13, 2020 at 10:16 PM Jungtaek Lim < >>> kabhwan.opensou...@gmail.com> wrote: >>> Hi devs, I know this is a super sensitive topic and at a risk of flame, but just like to try this. My apologies first. Assuming we all know about the ASF policy about code commit and I don't see Spark project has any explicit BYLAWS, it's technically possible to do anything for committers to do during merging. Sometimes this goes a bit depressing for reviewers, regardless of the intention, when merger makes a judgement by oneself to merge while the reviewers are still in the review phase. I observed the practice is used frequently, under the fact that we have post-review to address further comments later. I know about the concern that it's sometimes blocking unintentionally if we require merger to gather consensus about the merge from reviewers, but we also have some other practice holding on merging for a couple of days and noticing to reviewers whether they have further comments or not, which is I think a good trade-off. Exclude the cases where we're in release blocker mode, wouldn't we be hurt too much if we ask merger to respect the practice on noticing to reviewers that merging will be happen soon and waiting a day or so? I feel the post-review is opening the possibility for reviewers late on the party to review later, but it's over-used if it is leveraged as a judgement that merger can merge at any time and reviewers can still continue reviewing. Reviewers would feel broken flow - that is not the same experience with having more time to finalize reviewing before merging. Again I know it's super hard to reconsider the ongoing practice while the project has gone for the long way (10 years), but just wanted to hear the voices about this. Thanks, Jungtaek Lim (HeartSaVioR) >>>
Re: [DISCUSS] Review/merge phase, and post-review
I see some voices that it's not sufficient to understand the topic. Let me elaborate this a bit more. 1. There're multiple reviewers reviewing the PR. (Say, A, B, C, D) 2. A and B leaves review comments on the PR, but no one makes the explicit indication that these review comments are the final one. 3. The author of the PR addresses the review comments. 4. C checks that the review comments from A and B are addressed, and merges the PR. In parallel (or a bit later), A is trying to check whether the review comments are addressed (or even more, A could provide more review comments afterwards), and realized the PR is already merged. Saying again, there's "technically" no incorrect point. Let's give another example of what I said "trade-off". 1. There're multiple reviewers reviewing the PR. (Say, A, B, C, D) 2. A and B leaves review comments on the PR, but no one makes the explicit indication that these review comments are the final one. 3. The author of the PR addresses the review comments. 4. C checks that the review comments from A and B are addressed, and asks A and B to confirm whether there's no further review comments, with the condition that it will be merged in a few days later if there's no further feedback. 5. If A and B confirms or A and B doesn't provide new feedback in the period, C merges the PR. If A or B provides new feedback, go back to 3 with resetting the days. This is what we tend to comment as "@A @B I'll leave this a few days more to see if anyone has further comments. Otherwise I'll merge this.". I see both are used across various PRs, so it's not really something I want to blame. Just want to make us think about what would be the ideal approach we'd be better to prefer. On Sat, Nov 14, 2020 at 3:46 PM Jungtaek Lim wrote: > Oh sorry that was gone with flame (please just consider it as my fault) > and I just removed all comments. > > Btw, when I always initiate discussions, I really do love to start > discussion "without" specific instances which tend to go blaming each > other. I understand it's not easy to discuss without taking examples, but > I'll try to explain the situation on my best instead. Please let me know if > there's some ambiguous or unclear thing to think about. > > On Sat, Nov 14, 2020 at 3:41 PM Sean Owen wrote: > >> I am sure you are referring to some specific instances but I have not >> followed enough to know what they are. Can you point them out? I think that >> is most productive for everyone to understand. >> >> On Fri, Nov 13, 2020 at 10:16 PM Jungtaek Lim < >> kabhwan.opensou...@gmail.com> wrote: >> >>> Hi devs, >>> >>> I know this is a super sensitive topic and at a risk of flame, but just >>> like to try this. My apologies first. >>> Assuming we all know about the ASF policy about code commit and I don't >>> see Spark project has any explicit BYLAWS, it's technically possible to do >>> anything for committers to do during merging. >>> >>> Sometimes this goes a bit depressing for reviewers, regardless of the >>> intention, when merger makes a judgement by oneself to merge while the >>> reviewers are still in the review phase. I observed the practice is used >>> frequently, under the fact that we have post-review to address further >>> comments later. >>> >>> I know about the concern that it's sometimes blocking unintentionally if >>> we require merger to gather consensus about the merge from reviewers, but >>> we also have some other practice holding on merging for a couple of days >>> and noticing to reviewers whether they have further comments or not, which >>> is I think a good trade-off. >>> >>> Exclude the cases where we're in release blocker mode, wouldn't we be >>> hurt too much if we ask merger to respect the practice on noticing to >>> reviewers that merging will be happen soon and waiting a day or so? I feel >>> the post-review is opening the possibility for reviewers late on the party >>> to review later, but it's over-used if it is leveraged as a judgement that >>> merger can merge at any time and reviewers can still continue reviewing. >>> Reviewers would feel broken flow - that is not the same experience with >>> having more time to finalize reviewing before merging. >>> >>> Again I know it's super hard to reconsider the ongoing practice while >>> the project has gone for the long way (10 years), but just wanted to hear >>> the voices about this. >>> >>> Thanks, >>> Jungtaek Lim (HeartSaVioR) >>> >>
Re: [DISCUSS] Review/merge phase, and post-review
Oh sorry that was gone with flame (please just consider it as my fault) and I just removed all comments. Btw, when I always initiate discussions, I really do love to start discussion "without" specific instances which tend to go blaming each other. I understand it's not easy to discuss without taking examples, but I'll try to explain the situation on my best instead. Please let me know if there's some ambiguous or unclear thing to think about. On Sat, Nov 14, 2020 at 3:41 PM Sean Owen wrote: > I am sure you are referring to some specific instances but I have not > followed enough to know what they are. Can you point them out? I think that > is most productive for everyone to understand. > > On Fri, Nov 13, 2020 at 10:16 PM Jungtaek Lim < > kabhwan.opensou...@gmail.com> wrote: > >> Hi devs, >> >> I know this is a super sensitive topic and at a risk of flame, but just >> like to try this. My apologies first. >> Assuming we all know about the ASF policy about code commit and I don't >> see Spark project has any explicit BYLAWS, it's technically possible to do >> anything for committers to do during merging. >> >> Sometimes this goes a bit depressing for reviewers, regardless of the >> intention, when merger makes a judgement by oneself to merge while the >> reviewers are still in the review phase. I observed the practice is used >> frequently, under the fact that we have post-review to address further >> comments later. >> >> I know about the concern that it's sometimes blocking unintentionally if >> we require merger to gather consensus about the merge from reviewers, but >> we also have some other practice holding on merging for a couple of days >> and noticing to reviewers whether they have further comments or not, which >> is I think a good trade-off. >> >> Exclude the cases where we're in release blocker mode, wouldn't we be >> hurt too much if we ask merger to respect the practice on noticing to >> reviewers that merging will be happen soon and waiting a day or so? I feel >> the post-review is opening the possibility for reviewers late on the party >> to review later, but it's over-used if it is leveraged as a judgement that >> merger can merge at any time and reviewers can still continue reviewing. >> Reviewers would feel broken flow - that is not the same experience with >> having more time to finalize reviewing before merging. >> >> Again I know it's super hard to reconsider the ongoing practice while the >> project has gone for the long way (10 years), but just wanted to hear the >> voices about this. >> >> Thanks, >> Jungtaek Lim (HeartSaVioR) >> >
Re: [DISCUSS] Review/merge phase, and post-review
I am sure you are referring to some specific instances but I have not followed enough to know what they are. Can you point them out? I think that is most productive for everyone to understand. On Fri, Nov 13, 2020 at 10:16 PM Jungtaek Lim wrote: > Hi devs, > > I know this is a super sensitive topic and at a risk of flame, but just > like to try this. My apologies first. > Assuming we all know about the ASF policy about code commit and I don't > see Spark project has any explicit BYLAWS, it's technically possible to do > anything for committers to do during merging. > > Sometimes this goes a bit depressing for reviewers, regardless of the > intention, when merger makes a judgement by oneself to merge while the > reviewers are still in the review phase. I observed the practice is used > frequently, under the fact that we have post-review to address further > comments later. > > I know about the concern that it's sometimes blocking unintentionally if > we require merger to gather consensus about the merge from reviewers, but > we also have some other practice holding on merging for a couple of days > and noticing to reviewers whether they have further comments or not, which > is I think a good trade-off. > > Exclude the cases where we're in release blocker mode, wouldn't we be hurt > too much if we ask merger to respect the practice on noticing to reviewers > that merging will be happen soon and waiting a day or so? I feel the > post-review is opening the possibility for reviewers late on the party to > review later, but it's over-used if it is leveraged as a judgement that > merger can merge at any time and reviewers can still continue reviewing. > Reviewers would feel broken flow - that is not the same experience with > having more time to finalize reviewing before merging. > > Again I know it's super hard to reconsider the ongoing practice while the > project has gone for the long way (10 years), but just wanted to hear the > voices about this. > > Thanks, > Jungtaek Lim (HeartSaVioR) >
