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 <kabhwan.opensou...@gmail.com> 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 <sro...@gmail.com> 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) >>>> >>>
