[DISCUSS] Be more patient with PR and patches in the review

[Henry Saputra]

HI All,

I'd like to bring up a bit concerning flow I am start seeing in the few PRs.

I see some PRs had been rush to commit without addressing ALL comments
in the PR review.
For latest example is the comments Till and I made about using Option
instead of null [1] for Max's PR.
It is responsibility of the PR creator to address comment raise up in
the PR before any commiter could merge it. No need to rush it.

Would like to see this more to make sure PRs' issue or concerns are addressed.

Thanks,

- Henry

[1]  https://github.com/apache/flink/pull/344

[Stephan Ewen]

Hey Henry!

For pull request 344, I merged it, because I had already built a fix on top
of it while discussion was going on.

Here is the commit that addresses actually all comments in the discussion
(plus a bit more)
https://github.com/apache/flink/commit/56b7f85b4f6d522765df19a9710a098092ccde56

It is applied two commits later than the pull request commit.
It is true that I forgot to mirror that back into teh discussion. My bad!

If you think that is happening for more pull requests, then please raise
the issue, because that certainly should not happen.

Greetings,
Stephan


On Thu, Feb 5, 2015 at 5:03 PM, Henry Saputra <[hidden email]>
wrote:

> HI All,
>
> I'd like to bring up a bit concerning flow I am start seeing in the few
> PRs.
>
> I see some PRs had been rush to commit without addressing ALL comments
> in the PR review.
> For latest example is the comments Till and I made about using Option
> instead of null [1] for Max's PR.
> It is responsibility of the PR creator to address comment raise up in
> the PR before any commiter could merge it. No need to rush it.
>
> Would like to see this more to make sure PRs' issue or concerns are
> addressed.
>
> Thanks,
>
> - Henry
>
> [1]  https://github.com/apache/flink/pull/344
>

[Henry Saputra]

Ah awesome, I do not about that, thanks for letting me know. Mea culpa from me.

I think I saw only couple cases but thought I raise the discussions
before I forgot =P

Thanks for addressing this so quickly, Stephan.

- Henry


On Thu, Feb 5, 2015 at 8:09 AM, Stephan Ewen <[hidden email]> wrote:

> Hey Henry!
>
> For pull request 344, I merged it, because I had already built a fix on top
> of it while discussion was going on.
>
> Here is the commit that addresses actually all comments in the discussion
> (plus a bit more)
> https://github.com/apache/flink/commit/56b7f85b4f6d522765df19a9710a098092ccde56
>
> It is applied two commits later than the pull request commit.
> It is true that I forgot to mirror that back into teh discussion. My bad!
>
> If you think that is happening for more pull requests, then please raise
> the issue, because that certainly should not happen.
>
> Greetings,
> Stephan
>
>
> On Thu, Feb 5, 2015 at 5:03 PM, Henry Saputra <[hidden email]>
> wrote:
>
>> HI All,
>>
>> I'd like to bring up a bit concerning flow I am start seeing in the few
>> PRs.
>>
>> I see some PRs had been rush to commit without addressing ALL comments
>> in the PR review.
>> For latest example is the comments Till and I made about using Option
>> instead of null [1] for Max's PR.
>> It is responsibility of the PR creator to address comment raise up in
>> the PR before any commiter could merge it. No need to rush it.
>>
>> Would like to see this more to make sure PRs' issue or concerns are
>> addressed.
>>
>> Thanks,
>>
>> - Henry
>>
>> [1]  https://github.com/apache/flink/pull/344
>>

[Max Michels]

Hi Henry,

I forgot to leave a message stating that I'm fine with Stephan's
changes that would soon be merged into the master. Stephan did not
push to the master immediately, so further comments could have been
made to the pull request.

It would have been more transparent if we had posted the relevant
commit. I actually just looked it up in his branch.

By the way, I absorbed Till and your feedback and will seriously
consider using alternatives to the null value.

Best regards,
Max

On Thu, Feb 5, 2015 at 5:18 PM, Henry Saputra <[hidden email]> wrote:

> Ah awesome, I do not about that, thanks for letting me know. Mea culpa from me.
>
> I think I saw only couple cases but thought I raise the discussions
> before I forgot =P
>
> Thanks for addressing this so quickly, Stephan.
>
> - Henry
>
>
> On Thu, Feb 5, 2015 at 8:09 AM, Stephan Ewen <[hidden email]> wrote:
>> Hey Henry!
>>
>> For pull request 344, I merged it, because I had already built a fix on top
>> of it while discussion was going on.
>>
>> Here is the commit that addresses actually all comments in the discussion
>> (plus a bit more)
>> https://github.com/apache/flink/commit/56b7f85b4f6d522765df19a9710a098092ccde56
>>
>> It is applied two commits later than the pull request commit.
>> It is true that I forgot to mirror that back into teh discussion. My bad!
>>
>> If you think that is happening for more pull requests, then please raise
>> the issue, because that certainly should not happen.
>>
>> Greetings,
>> Stephan
>>
>>
>> On Thu, Feb 5, 2015 at 5:03 PM, Henry Saputra <[hidden email]>
>> wrote:
>>
>>> HI All,
>>>
>>> I'd like to bring up a bit concerning flow I am start seeing in the few
>>> PRs.
>>>
>>> I see some PRs had been rush to commit without addressing ALL comments
>>> in the PR review.
>>> For latest example is the comments Till and I made about using Option
>>> instead of null [1] for Max's PR.
>>> It is responsibility of the PR creator to address comment raise up in
>>> the PR before any commiter could merge it. No need to rush it.
>>>
>>> Would like to see this more to make sure PRs' issue or concerns are
>>> addressed.
>>>
>>> Thanks,
>>>
>>> - Henry
>>>
>>> [1]  https://github.com/apache/flink/pull/344
>>>

[Henry Saputra]

Thanks Max, really appreciate it.

Stephan just pulled me aside and tell me that he had merge the changes
into separate branch.
Bit misunderstanding from my side, but I want to make sure Flink
community be awesome and great place for people to come in and
contribute.

Apologize for jumping the gun there.

- Henry

On Thu, Feb 5, 2015 at 8:26 AM, Max Michels <[hidden email]> wrote:

> Hi Henry,
>
> I forgot to leave a message stating that I'm fine with Stephan's
> changes that would soon be merged into the master. Stephan did not
> push to the master immediately, so further comments could have been
> made to the pull request.
>
> It would have been more transparent if we had posted the relevant
> commit. I actually just looked it up in his branch.
>
> By the way, I absorbed Till and your feedback and will seriously
> consider using alternatives to the null value.
>
> Best regards,
> Max
>
> On Thu, Feb 5, 2015 at 5:18 PM, Henry Saputra <[hidden email]> wrote:
>> Ah awesome, I do not about that, thanks for letting me know. Mea culpa from me.
>>
>> I think I saw only couple cases but thought I raise the discussions
>> before I forgot =P
>>
>> Thanks for addressing this so quickly, Stephan.
>>
>> - Henry
>>
>>
>> On Thu, Feb 5, 2015 at 8:09 AM, Stephan Ewen <[hidden email]> wrote:
>>> Hey Henry!
>>>
>>> For pull request 344, I merged it, because I had already built a fix on top
>>> of it while discussion was going on.
>>>
>>> Here is the commit that addresses actually all comments in the discussion
>>> (plus a bit more)
>>> https://github.com/apache/flink/commit/56b7f85b4f6d522765df19a9710a098092ccde56
>>>
>>> It is applied two commits later than the pull request commit.
>>> It is true that I forgot to mirror that back into teh discussion. My bad!
>>>
>>> If you think that is happening for more pull requests, then please raise
>>> the issue, because that certainly should not happen.
>>>
>>> Greetings,
>>> Stephan
>>>
>>>
>>> On Thu, Feb 5, 2015 at 5:03 PM, Henry Saputra <[hidden email]>
>>> wrote:
>>>
>>>> HI All,
>>>>
>>>> I'd like to bring up a bit concerning flow I am start seeing in the few
>>>> PRs.
>>>>
>>>> I see some PRs had been rush to commit without addressing ALL comments
>>>> in the PR review.
>>>> For latest example is the comments Till and I made about using Option
>>>> instead of null [1] for Max's PR.
>>>> It is responsibility of the PR creator to address comment raise up in
>>>> the PR before any commiter could merge it. No need to rush it.
>>>>
>>>> Would like to see this more to make sure PRs' issue or concerns are
>>>> addressed.
>>>>
>>>> Thanks,
>>>>
>>>> - Henry
>>>>
>>>> [1]  https://github.com/apache/flink/pull/344
>>>>