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, <mri...@gmail.com> 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 <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) >>>>> >>>>
