spark-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mridul Muralidharan <mri...@gmail.com>
Subject Re: [DISCUSS] Review/merge phase, and post-review
Date Sat, 14 Nov 2020 07:53:08 GMT
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.opensource@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.opensource@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 <srowen@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.opensource@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)
>>>>
>>>

Mime
View raw message