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) >>> >>
