(DEPRECATED) Apache Flink Mailing List archive.

Revert 78fd2146dd until we have consensus for FLINK-2419?

Classic

List

Threaded

10 messages Options

Robert Metzger

Revert 78fd2146dd until we have consensus for FLINK-2419?

Hi,

I'm a bit unhappy how we were handling
https://issues.apache.org/jira/browse/FLINK-2419 today.

I raised a concern in the JIRA because the commit for the fix didn't
contain any tests. Our coding guidelines [1] imply that every feature
should have tests. Apparently there were not enough tests for the two bugs
fixed with commit 78fd2146dd.

Also, Gyula's answer sounds like he is not willing to add tests right now.

I can not remember if we ever reverted a commit in the Flink community, but
in my understanding, this is how ASF projects are doing lazy consensus for
commits-without-PR.
So if there is a disagreement in the associated JIRA, we revert the fix
until there is an agreement.

In this case, I did not immediately revert the commit, because I would like
to see whether others in the community agree with me.

What do you think how we should handle cases like this one in the future?

I think its very important for committers and PMC members to be a good
example when it comes to following our own rules. Otherwise, how can we ask
our contributors to adhere to these rules?

My suggestion to resolve this situation is the following:
- Revert commit 78fd2146dd
- open pull requests for FLINK-2419 and FLINK-2423 (with tests of course),
review and merge them.

Best,
Robert

[1] http://flink.apache.org/coding-guidelines.html

Kostas Tzoumas-2

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

I'm probably lacking a bit of context, but by reading your conversation at
JIRA it seems to me that commit

https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7

does not contain a test, and Robert is asking for a test which means that
we do not have consensus. If this was a pull request and not a commit it
would not be merged until consensus was reached, so my opinion is that the
same should happen now: the commit should be reverted, test(s) should be
added, and then merged again (or better submitted as a PR). More so as this
is deep in the core system code.

Kostas

On Tue, Jul 28, 2015 at 8:01 PM, Robert Metzger <[hidden email]> wrote:

> Hi,
>
> I'm a bit unhappy how we were handling
> https://issues.apache.org/jira/browse/FLINK-2419 today.
>
> I raised a concern in the JIRA because the commit for the fix didn't
> contain any tests. Our coding guidelines [1] imply that every feature
> should have tests. Apparently there were not enough tests for the two bugs
> fixed with commit 78fd2146dd.
>
> Also, Gyula's answer sounds like he is not willing to add tests right now.
>
> I can not remember if we ever reverted a commit in the Flink community, but
> in my understanding, this is how ASF projects are doing lazy consensus for
> commits-without-PR.
> So if there is a disagreement in the associated JIRA, we revert the fix
> until there is an agreement.
>
> In this case, I did not immediately revert the commit, because I would like
> to see whether others in the community agree with me.
>
>
> What do you think how we should handle cases like this one in the future?
>
> I think its very important for committers and PMC members to be a good
> example when it comes to following our own rules. Otherwise, how can we ask
> our contributors to adhere to these rules?
>
>
> My suggestion to resolve this situation is the following:
> - Revert commit 78fd2146dd
> - open pull requests for FLINK-2419 and FLINK-2423 (with tests of course),
> review and merge them.
>
>
>
> Best,
> Robert
>
> [1] http://flink.apache.org/coding-guidelines.html
>

Gyula Fóra

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

In reply to this post by Robert Metzger

Hey,

I am sorry that you feel bad about this, I only did not add a test
case for FLINK-2419
because I am adding a test in my upcoming PR which verified the behaviour.

As for FLINK-2423, it is actually very bad that issue is still there. You
introduced this in your PR https://github.com/apache/flink/pull/895 which I
commented but no one fixed it before merging. As developing a test takes
quite much time here as it is tricky, I wanted to push the fix, which was
in fact trivial.

Regards,
Gyula

Robert Metzger <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
20:01):

Gyula Fóra

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

What concerns me here is that for FLINK-2419 I clearly indicated that there
is a test in my other PR, and since the fix was actually trivial, which
didn't break the current functionality according my test, I wanted to push
it in before my PR because that is pending on something else. I could have
added a test here that is true.

With FLINK-2423 I was fixing some else's mistake who disregarded my message
when merging a PR. We could now revert that PR that introduced that bug,
but instead we are reverting my fix for that mistake.

Gyula Fóra <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
20:19):

> Hey,
>
> I am sorry that you feel bad about this, I only did not add a test case
> for FLINK-2419 because I am adding a test in my upcoming PR which
> verified the behaviour.
>
> As for FLINK-2423, it is actually very bad that issue is still there. You
> introduced this in your PR https://github.com/apache/flink/pull/895 which
> I commented but no one fixed it before merging. As developing a test takes
> quite much time here as it is tricky, I wanted to push the fix, which was
> in fact trivial.
>
> Regards,
> Gyula
>
> Robert Metzger <[hidden email]> ezt írta (időpont: 2015. júl. 28.,
> K, 20:01):
>
>> Hi,
>>
>> I'm a bit unhappy how we were handling
>> https://issues.apache.org/jira/browse/FLINK-2419 today.
>>
>> I raised a concern in the JIRA because the commit for the fix didn't
>> contain any tests. Our coding guidelines [1] imply that every feature
>> should have tests. Apparently there were not enough tests for the two bugs
>> fixed with commit 78fd2146dd.
>>
>> Also, Gyula's answer sounds like he is not willing to add tests right now.
>>
>> I can not remember if we ever reverted a commit in the Flink community,
>> but
>> in my understanding, this is how ASF projects are doing lazy consensus for
>> commits-without-PR.
>> So if there is a disagreement in the associated JIRA, we revert the fix
>> until there is an agreement.
>>
>> In this case, I did not immediately revert the commit, because I would
>> like
>> to see whether others in the community agree with me.
>>
>>
>> What do you think how we should handle cases like this one in the future?
>>
>> I think its very important for committers and PMC members to be a good
>> example when it comes to following our own rules. Otherwise, how can we
>> ask
>> our contributors to adhere to these rules?
>>
>>
>> My suggestion to resolve this situation is the following:
>> - Revert commit 78fd2146dd
>> - open pull requests for FLINK-2419 and FLINK-2423 (with tests of course),
>> review and merge them.
>>
>>
>>
>> Best,
>> Robert
>>
>> [1] http://flink.apache.org/coding-guidelines.html
>>
>

Kostas Tzoumas-2

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

I am not familiar with this part of the code, but this is perhaps a good
thing, as this is a matter of policy, not who introduced which bug (I
suspect that the policy issue was Robert's motivation for starting a thread
at the dev list)

So, I think we have two issues:

(1) Pull request https://github.com/apache/flink/pull/895 was merged
without addressing Gyula's comment.

(2) Commit
https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7
was
merged but consensus was not reached.

Let's keep the two issues separate, as tracing back whose bug a PR is
fixing (recursively :-) will not lead anywhere.

Now, back to the original question: I think that commits should be subject
to consensus in a similar way as PRs. The right to commit does not mean
that consensus should not be reached, and this is a clear case of not
having consensus.

Kostas

On Tue, Jul 28, 2015 at 8:30 PM, Gyula Fóra <[hidden email]> wrote:

> What concerns me here is that for FLINK-2419 I clearly indicated that there
> is a test in my other PR, and since the fix was actually trivial, which
> didn't break the current functionality according my test, I wanted to push
> it in before my PR because that is pending on something else. I could have
> added a test here that is true.
>
> With FLINK-2423 I was fixing some else's mistake who disregarded my message
> when merging a PR. We could now revert that PR that introduced that bug,
> but instead we are reverting my fix for that mistake.
>
>
>
>
> Gyula Fóra <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
> 20:19):
>
> > Hey,
> >
> > I am sorry that you feel bad about this, I only did not add a test case
> > for FLINK-2419 because I am adding a test in my upcoming PR which
> > verified the behaviour.
> >
> > As for FLINK-2423, it is actually very bad that issue is still there. You
> > introduced this in your PR https://github.com/apache/flink/pull/895
> which
> > I commented but no one fixed it before merging. As developing a test
> takes
> > quite much time here as it is tricky, I wanted to push the fix, which was
> > in fact trivial.
> >
> > Regards,
> > Gyula
> >
> > Robert Metzger <[hidden email]> ezt írta (időpont: 2015. júl. 28.,
> > K, 20:01):
> >
> >> Hi,
> >>
> >> I'm a bit unhappy how we were handling
> >> https://issues.apache.org/jira/browse/FLINK-2419 today.
> >>
> >> I raised a concern in the JIRA because the commit for the fix didn't
> >> contain any tests. Our coding guidelines [1] imply that every feature
> >> should have tests. Apparently there were not enough tests for the two
> bugs
> >> fixed with commit 78fd2146dd.
> >>
> >> Also, Gyula's answer sounds like he is not willing to add tests right
> now.
> >>
> >> I can not remember if we ever reverted a commit in the Flink community,
> >> but
> >> in my understanding, this is how ASF projects are doing lazy consensus
> for
> >> commits-without-PR.
> >> So if there is a disagreement in the associated JIRA, we revert the fix
> >> until there is an agreement.
> >>
> >> In this case, I did not immediately revert the commit, because I would
> >> like
> >> to see whether others in the community agree with me.
> >>
> >>
> >> What do you think how we should handle cases like this one in the
> future?
> >>
> >> I think its very important for committers and PMC members to be a good
> >> example when it comes to following our own rules. Otherwise, how can we
> >> ask
> >> our contributors to adhere to these rules?
> >>
> >>
> >> My suggestion to resolve this situation is the following:
> >> - Revert commit 78fd2146dd
> >> - open pull requests for FLINK-2419 and FLINK-2423 (with tests of
> course),
> >> review and merge them.
> >>
> >>
> >>
> >> Best,
> >> Robert
> >>
> >> [1] http://flink.apache.org/coding-guidelines.html
> >>
> >
>

Gyula Fóra

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

I agree that consensus should be reached in all changes to the system.

What is not clear to me is what is the subject of consensus in this case.

As for FLINK-2423, this is clearly an issue, and the only question here is
whether my solution solves it or not. I think it is fair to say that this
is a trivial fix for this issue, but in any case it should be tested. I had
two options: fix it without a test, fix it and add a test. Unfortunately I
did not have time to add a test because I was busy with other things but I
did not want to leave that bug in. We can revert the commit back which will
still not give me time to write the test so the bug will potentially remain
there for long (as Robert indicated he doesnt have time for it either).

As for FLINK-2419, I agree that I could have added a simple test which
would not have taken me long. The only reason I did this because I am
testing this functionality in another PR. I understand if you want to
revert this I can open a PR with the simple test added.

Would it have been better if I did not address the first issue if I dont
have a time to write a proper test? Then that issue would have been
lingering there in a core functionality for who knows how long.

I would like to clearly understand what is expected in this situation.

Gyula

Kostas Tzoumas <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
20:48):

> I am not familiar with this part of the code, but this is perhaps a good
> thing, as this is a matter of policy, not who introduced which bug (I
> suspect that the policy issue was Robert's motivation for starting a thread
> at the dev list)
>
> So, I think we have two issues:
>
> (1) Pull request https://github.com/apache/flink/pull/895 was merged
> without addressing Gyula's comment.
>
> (2) Commit
>
> https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7
> was
> merged but consensus was not reached.
>
> Let's keep the two issues separate, as tracing back whose bug a PR is
> fixing (recursively :-) will not lead anywhere.
>
> Now, back to the original question: I think that commits should be subject
> to consensus in a similar way as PRs. The right to commit does not mean
> that consensus should not be reached, and this is a clear case of not
> having consensus.
>
> Kostas
>
>
> On Tue, Jul 28, 2015 at 8:30 PM, Gyula Fóra <[hidden email]> wrote:
>
> > What concerns me here is that for FLINK-2419 I clearly indicated that
> there
> > is a test in my other PR, and since the fix was actually trivial, which
> > didn't break the current functionality according my test, I wanted to
> push
> > it in before my PR because that is pending on something else. I could
> have
> > added a test here that is true.
> >
> > With FLINK-2423 I was fixing some else's mistake who disregarded my
> message
> > when merging a PR. We could now revert that PR that introduced that bug,
> > but instead we are reverting my fix for that mistake.
> >
> >
> >
> >
> > Gyula Fóra <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
> > 20:19):
> >
> > > Hey,
> > >
> > > I am sorry that you feel bad about this, I only did not add a test case
> > > for FLINK-2419 because I am adding a test in my upcoming PR which
> > > verified the behaviour.
> > >
> > > As for FLINK-2423, it is actually very bad that issue is still there.
> You
> > > introduced this in your PR https://github.com/apache/flink/pull/895
> > which
> > > I commented but no one fixed it before merging. As developing a test
> > takes
> > > quite much time here as it is tricky, I wanted to push the fix, which
> was
> > > in fact trivial.
> > >
> > > Regards,
> > > Gyula
> > >
> > > Robert Metzger <[hidden email]> ezt írta (időpont: 2015. júl.
> 28.,
> > > K, 20:01):
> > >
> > >> Hi,
> > >>
> > >> I'm a bit unhappy how we were handling
> > >> https://issues.apache.org/jira/browse/FLINK-2419 today.
> > >>
> > >> I raised a concern in the JIRA because the commit for the fix didn't
> > >> contain any tests. Our coding guidelines [1] imply that every feature
> > >> should have tests. Apparently there were not enough tests for the two
> > bugs
> > >> fixed with commit 78fd2146dd.
> > >>
> > >> Also, Gyula's answer sounds like he is not willing to add tests right
> > now.
> > >>
> > >> I can not remember if we ever reverted a commit in the Flink
> community,
> > >> but
> > >> in my understanding, this is how ASF projects are doing lazy consensus
> > for
> > >> commits-without-PR.
> > >> So if there is a disagreement in the associated JIRA, we revert the
> fix
> > >> until there is an agreement.
> > >>
> > >> In this case, I did not immediately revert the commit, because I would
> > >> like
> > >> to see whether others in the community agree with me.
> > >>
> > >>
> > >> What do you think how we should handle cases like this one in the
> > future?
> > >>
> > >> I think its very important for committers and PMC members to be a good
> > >> example when it comes to following our own rules. Otherwise, how can
> we
> > >> ask
> > >> our contributors to adhere to these rules?
> > >>
> > >>
> > >> My suggestion to resolve this situation is the following:
> > >> - Revert commit 78fd2146dd
> > >> - open pull requests for FLINK-2419 and FLINK-2423 (with tests of
> > course),
> > >> review and merge them.
> > >>
> > >>
> > >>
> > >> Best,
> > >> Robert
> > >>
> > >> [1] http://flink.apache.org/coding-guidelines.html
> > >>
> > >
> >
>

Kostas Tzoumas-2

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

On Tue, Jul 28, 2015 at 9:09 PM, Gyula Fóra <[hidden email]> wrote:

> I agree that consensus should be reached in all changes to the system.
>
>
Then Robert and you should reach consensus on FLINK-2419.

> What is not clear to me is what is the subject of consensus in this case.
>
> As for FLINK-2423, this is clearly an issue, and the only question here is
> whether my solution solves it or not. I think it is fair to say that this
> is a trivial fix for this issue, but in any case it should be tested. I had
> two options: fix it without a test, fix it and add a test. Unfortunately I
> did not have time to add a test because I was busy with other things but I
> did not want to leave that bug in. We can revert the commit back which will
> still not give me time to write the test so the bug will potentially remain
> there for long (as Robert indicated he doesnt have time for it either).
>
> As for FLINK-2419, I agree that I could have added a simple test which
> would not have taken me long. The only reason I did this because I am
> testing this functionality in another PR. I understand if you want to
> revert this I can open a PR with the simple test added.
>
> Would it have been better if I did not address the first issue if I dont
> have a time to write a proper test? Then that issue would have been
> lingering there in a core functionality for who knows how long.
>
> I would like to clearly understand what is expected in this situation.
>
>

Well, we get to define what is expected, that's the fun of being open
source :-) In my opinion it is better to provide a well tested fix later
than a potentially sloppy fix earlier.

> Gyula
>
>
> Kostas Tzoumas <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
> 20:48):
>
> > I am not familiar with this part of the code, but this is perhaps a good
> > thing, as this is a matter of policy, not who introduced which bug (I
> > suspect that the policy issue was Robert's motivation for starting a
> thread
> > at the dev list)
> >
> > So, I think we have two issues:
> >
> > (1) Pull request https://github.com/apache/flink/pull/895 was merged
> > without addressing Gyula's comment.
> >
> > (2) Commit
> >
> >
> https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7
> > was
> > merged but consensus was not reached.
> >
> > Let's keep the two issues separate, as tracing back whose bug a PR is
> > fixing (recursively :-) will not lead anywhere.
> >
> > Now, back to the original question: I think that commits should be
> subject
> > to consensus in a similar way as PRs. The right to commit does not mean
> > that consensus should not be reached, and this is a clear case of not
> > having consensus.
> >
> > Kostas
> >
> >
> > On Tue, Jul 28, 2015 at 8:30 PM, Gyula Fóra <[hidden email]>
> wrote:
> >
> > > What concerns me here is that for FLINK-2419 I clearly indicated that
> > there
> > > is a test in my other PR, and since the fix was actually trivial, which
> > > didn't break the current functionality according my test, I wanted to
> > push
> > > it in before my PR because that is pending on something else. I could
> > have
> > > added a test here that is true.
> > >
> > > With FLINK-2423 I was fixing some else's mistake who disregarded my
> > message
> > > when merging a PR. We could now revert that PR that introduced that
> bug,
> > > but instead we are reverting my fix for that mistake.
> > >
> > >
> > >
> > >
> > > Gyula Fóra <[hidden email]> ezt írta (időpont: 2015. júl. 28.,
> K,
> > > 20:19):
> > >
> > > > Hey,
> > > >
> > > > I am sorry that you feel bad about this, I only did not add a test
> case
> > > > for FLINK-2419 because I am adding a test in my upcoming PR which
> > > > verified the behaviour.
> > > >
> > > > As for FLINK-2423, it is actually very bad that issue is still there.
> > You
> > > > introduced this in your PR https://github.com/apache/flink/pull/895
> > > which
> > > > I commented but no one fixed it before merging. As developing a test
> > > takes
> > > > quite much time here as it is tricky, I wanted to push the fix, which
> > was
> > > > in fact trivial.
> > > >
> > > > Regards,
> > > > Gyula
> > > >
> > > > Robert Metzger <[hidden email]> ezt írta (időpont: 2015. júl.
> > 28.,
> > > > K, 20:01):
> > > >
> > > >> Hi,
> > > >>
> > > >> I'm a bit unhappy how we were handling
> > > >> https://issues.apache.org/jira/browse/FLINK-2419 today.
> > > >>
> > > >> I raised a concern in the JIRA because the commit for the fix didn't
> > > >> contain any tests. Our coding guidelines [1] imply that every
> feature
> > > >> should have tests. Apparently there were not enough tests for the
> two
> > > bugs
> > > >> fixed with commit 78fd2146dd.
> > > >>
> > > >> Also, Gyula's answer sounds like he is not willing to add tests
> right
> > > now.
> > > >>
> > > >> I can not remember if we ever reverted a commit in the Flink
> > community,
> > > >> but
> > > >> in my understanding, this is how ASF projects are doing lazy
> consensus
> > > for
> > > >> commits-without-PR.
> > > >> So if there is a disagreement in the associated JIRA, we revert the
> > fix
> > > >> until there is an agreement.
> > > >>
> > > >> In this case, I did not immediately revert the commit, because I
> would
> > > >> like
> > > >> to see whether others in the community agree with me.
> > > >>
> > > >>
> > > >> What do you think how we should handle cases like this one in the
> > > future?
> > > >>
> > > >> I think its very important for committers and PMC members to be a
> good
> > > >> example when it comes to following our own rules. Otherwise, how can
> > we
> > > >> ask
> > > >> our contributors to adhere to these rules?
> > > >>
> > > >>
> > > >> My suggestion to resolve this situation is the following:
> > > >> - Revert commit 78fd2146dd
> > > >> - open pull requests for FLINK-2419 and FLINK-2423 (with tests of
> > > course),
> > > >> review and merge them.
> > > >>
> > > >>
> > > >>
> > > >> Best,
> > > >> Robert
> > > >>
> > > >> [1] http://flink.apache.org/coding-guidelines.html
> > > >>
> > > >
> > >
> >
>

Gyula Fóra

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

Hey,

I think there is no reason for making a more serious issue out of this than
it already is :)

I have opened a pull request that adds the missing test for FLINK-2419:
https://github.com/apache/flink/pull/947
There everyone can verify that my commit has actually fixed the problem.
This should have been included in the commit itself.

If the community decides so, I am comfortable with reverting the commit, in
which case I will add the solution to FLINK-2419 to the PR mentioned above
and can be merged as one commit. Unfortunately I do not have time to
develop a thorough test for FLINK-2423 so in case we revert the commit I
have to exclude the bugfix for that issue, and I encourage anyone to pick
up that test case (https://issues.apache.org/jira/browse/FLINK-2423).

Regards,
Gyula

Kostas Tzoumas <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
21:36):

> On Tue, Jul 28, 2015 at 9:09 PM, Gyula Fóra <[hidden email]> wrote:
>
> > I agree that consensus should be reached in all changes to the system.
> >
> >
> Then Robert and you should reach consensus on FLINK-2419.
>
>
> > What is not clear to me is what is the subject of consensus in this case.
> >
> > As for FLINK-2423, this is clearly an issue, and the only question here
> is
> > whether my solution solves it or not. I think it is fair to say that this
> > is a trivial fix for this issue, but in any case it should be tested. I
> had
> > two options: fix it without a test, fix it and add a test. Unfortunately
> I
> > did not have time to add a test because I was busy with other things but
> I
> > did not want to leave that bug in. We can revert the commit back which
> will
> > still not give me time to write the test so the bug will potentially
> remain
> > there for long (as Robert indicated he doesnt have time for it either).
> >
> > As for FLINK-2419, I agree that I could have added a simple test which
> > would not have taken me long. The only reason I did this because I am
> > testing this functionality in another PR. I understand if you want to
> > revert this I can open a PR with the simple test added.
> >
> > Would it have been better if I did not address the first issue if I dont
> > have a time to write a proper test? Then that issue would have been
> > lingering there in a core functionality for who knows how long.
> >
> > I would like to clearly understand what is expected in this situation.
> >
> >
> Well, we get to define what is expected, that's the fun of being open
> source :-) In my opinion it is better to provide a well tested fix later
> than a potentially sloppy fix earlier.
>
>
>
> > Gyula
> >
> >
> > Kostas Tzoumas <[hidden email]> ezt írta (időpont: 2015. júl. 28.,
> K,
> > 20:48):
> >
> > > I am not familiar with this part of the code, but this is perhaps a
> good
> > > thing, as this is a matter of policy, not who introduced which bug (I
> > > suspect that the policy issue was Robert's motivation for starting a
> > thread
> > > at the dev list)
> > >
> > > So, I think we have two issues:
> > >
> > > (1) Pull request https://github.com/apache/flink/pull/895 was merged
> > > without addressing Gyula's comment.
> > >
> > > (2) Commit
> > >
> > >
> >
> https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7
> > > was
> > > merged but consensus was not reached.
> > >
> > > Let's keep the two issues separate, as tracing back whose bug a PR is
> > > fixing (recursively :-) will not lead anywhere.
> > >
> > > Now, back to the original question: I think that commits should be
> > subject
> > > to consensus in a similar way as PRs. The right to commit does not mean
> > > that consensus should not be reached, and this is a clear case of not
> > > having consensus.
> > >
> > > Kostas
> > >
> > >
> > > On Tue, Jul 28, 2015 at 8:30 PM, Gyula Fóra <[hidden email]>
> > wrote:
> > >
> > > > What concerns me here is that for FLINK-2419 I clearly indicated that
> > > there
> > > > is a test in my other PR, and since the fix was actually trivial,
> which
> > > > didn't break the current functionality according my test, I wanted to
> > > push
> > > > it in before my PR because that is pending on something else. I could
> > > have
> > > > added a test here that is true.
> > > >
> > > > With FLINK-2423 I was fixing some else's mistake who disregarded my
> > > message
> > > > when merging a PR. We could now revert that PR that introduced that
> > bug,
> > > > but instead we are reverting my fix for that mistake.
> > > >
> > > >
> > > >
> > > >
> > > > Gyula Fóra <[hidden email]> ezt írta (időpont: 2015. júl. 28.,
> > K,
> > > > 20:19):
> > > >
> > > > > Hey,
> > > > >
> > > > > I am sorry that you feel bad about this, I only did not add a test
> > case
> > > > > for FLINK-2419 because I am adding a test in my upcoming PR which
> > > > > verified the behaviour.
> > > > >
> > > > > As for FLINK-2423, it is actually very bad that issue is still
> there.
> > > You
> > > > > introduced this in your PR
> https://github.com/apache/flink/pull/895
> > > > which
> > > > > I commented but no one fixed it before merging. As developing a
> test
> > > > takes
> > > > > quite much time here as it is tricky, I wanted to push the fix,
> which
> > > was
> > > > > in fact trivial.
> > > > >
> > > > > Regards,
> > > > > Gyula
> > > > >
> > > > > Robert Metzger <[hidden email]> ezt írta (időpont: 2015. júl.
> > > 28.,
> > > > > K, 20:01):
> > > > >
> > > > >> Hi,
> > > > >>
> > > > >> I'm a bit unhappy how we were handling
> > > > >> https://issues.apache.org/jira/browse/FLINK-2419 today.
> > > > >>
> > > > >> I raised a concern in the JIRA because the commit for the fix
> didn't
> > > > >> contain any tests. Our coding guidelines [1] imply that every
> > feature
> > > > >> should have tests. Apparently there were not enough tests for the
> > two
> > > > bugs
> > > > >> fixed with commit 78fd2146dd.
> > > > >>
> > > > >> Also, Gyula's answer sounds like he is not willing to add tests
> > right
> > > > now.
> > > > >>
> > > > >> I can not remember if we ever reverted a commit in the Flink
> > > community,
> > > > >> but
> > > > >> in my understanding, this is how ASF projects are doing lazy
> > consensus
> > > > for
> > > > >> commits-without-PR.
> > > > >> So if there is a disagreement in the associated JIRA, we revert
> the
> > > fix
> > > > >> until there is an agreement.
> > > > >>
> > > > >> In this case, I did not immediately revert the commit, because I
> > would
> > > > >> like
> > > > >> to see whether others in the community agree with me.
> > > > >>
> > > > >>
> > > > >> What do you think how we should handle cases like this one in the
> > > > future?
> > > > >>
> > > > >> I think its very important for committers and PMC members to be a
> > good
> > > > >> example when it comes to following our own rules. Otherwise, how
> can
> > > we
> > > > >> ask
> > > > >> our contributors to adhere to these rules?
> > > > >>
> > > > >>
> > > > >> My suggestion to resolve this situation is the following:
> > > > >> - Revert commit 78fd2146dd
> > > > >> - open pull requests for FLINK-2419 and FLINK-2423 (with tests of
> > > > course),
> > > > >> review and merge them.
> > > > >>
> > > > >>
> > > > >>
> > > > >> Best,
> > > > >> Robert
> > > > >>
> > > > >> [1] http://flink.apache.org/coding-guidelines.html
> > > > >>
> > > > >
> > > >
> > >
> >
>

Fabian Hueske-2

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

Sounds reasonable to me.

In addition to the test-inclusion guideline, we should also pay more
attention to our single-change-per-PR rule (or commit in case of push
without PR) to avoid discussions about unrelated changes in the future.

Cheers, Fabian

2015-07-28 22:44 GMT+02:00 Gyula Fóra <[hidden email]>:

> Hey,
>
> I think there is no reason for making a more serious issue out of this than
> it already is :)
>
> I have opened a pull request that adds the missing test for FLINK-2419:
> https://github.com/apache/flink/pull/947
> There everyone can verify that my commit has actually fixed the problem.
> This should have been included in the commit itself.
>
> If the community decides so, I am comfortable with reverting the commit, in
> which case I will add the solution to FLINK-2419 to the PR mentioned above
> and can be merged as one commit. Unfortunately I do not have time to
> develop a thorough test for FLINK-2423 so in case we revert the commit I
> have to exclude the bugfix for that issue, and I encourage anyone to pick
> up that test case (https://issues.apache.org/jira/browse/FLINK-2423).
>
> Regards,
> Gyula
>
> Kostas Tzoumas <[hidden email]> ezt írta (időpont: 2015. júl. 28., K,
> 21:36):
>
> > On Tue, Jul 28, 2015 at 9:09 PM, Gyula Fóra <[hidden email]>
> wrote:
> >
> > > I agree that consensus should be reached in all changes to the system.
> > >
> > >
> > Then Robert and you should reach consensus on FLINK-2419.
> >
> >
> > > What is not clear to me is what is the subject of consensus in this
> case.
> > >
> > > As for FLINK-2423, this is clearly an issue, and the only question here
> > is
> > > whether my solution solves it or not. I think it is fair to say that
> this
> > > is a trivial fix for this issue, but in any case it should be tested. I
> > had
> > > two options: fix it without a test, fix it and add a test.
> Unfortunately
> > I
> > > did not have time to add a test because I was busy with other things
> but
> > I
> > > did not want to leave that bug in. We can revert the commit back which
> > will
> > > still not give me time to write the test so the bug will potentially
> > remain
> > > there for long (as Robert indicated he doesnt have time for it either).
> > >
> > > As for FLINK-2419, I agree that I could have added a simple test which
> > > would not have taken me long. The only reason I did this because I am
> > > testing this functionality in another PR. I understand if you want to
> > > revert this I can open a PR with the simple test added.
> > >
> > > Would it have been better if I did not address the first issue if I
> dont
> > > have a time to write a proper test? Then that issue would have been
> > > lingering there in a core functionality for who knows how long.
> > >
> > > I would like to clearly understand what is expected in this situation.
> > >
> > >
> > Well, we get to define what is expected, that's the fun of being open
> > source :-) In my opinion it is better to provide a well tested fix later
> > than a potentially sloppy fix earlier.
> >
> >
> >
> > > Gyula
> > >
> > >
> > > Kostas Tzoumas <[hidden email]> ezt írta (időpont: 2015. júl.
> 28.,
> > K,
> > > 20:48):
> > >
> > > > I am not familiar with this part of the code, but this is perhaps a
> > good
> > > > thing, as this is a matter of policy, not who introduced which bug (I
> > > > suspect that the policy issue was Robert's motivation for starting a
> > > thread
> > > > at the dev list)
> > > >
> > > > So, I think we have two issues:
> > > >
> > > > (1) Pull request https://github.com/apache/flink/pull/895 was merged
> > > > without addressing Gyula's comment.
> > > >
> > > > (2) Commit
> > > >
> > > >
> > >
> >
> https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7
> > > > was
> > > > merged but consensus was not reached.
> > > >
> > > > Let's keep the two issues separate, as tracing back whose bug a PR is
> > > > fixing (recursively :-) will not lead anywhere.
> > > >
> > > > Now, back to the original question: I think that commits should be
> > > subject
> > > > to consensus in a similar way as PRs. The right to commit does not
> mean
> > > > that consensus should not be reached, and this is a clear case of not
> > > > having consensus.
> > > >
> > > > Kostas
> > > >
> > > >
> > > > On Tue, Jul 28, 2015 at 8:30 PM, Gyula Fóra <[hidden email]>
> > > wrote:
> > > >
> > > > > What concerns me here is that for FLINK-2419 I clearly indicated
> that
> > > > there
> > > > > is a test in my other PR, and since the fix was actually trivial,
> > which
> > > > > didn't break the current functionality according my test, I wanted
> to
> > > > push
> > > > > it in before my PR because that is pending on something else. I
> could
> > > > have
> > > > > added a test here that is true.
> > > > >
> > > > > With FLINK-2423 I was fixing some else's mistake who disregarded my
> > > > message
> > > > > when merging a PR. We could now revert that PR that introduced that
> > > bug,
> > > > > but instead we are reverting my fix for that mistake.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Gyula Fóra <[hidden email]> ezt írta (időpont: 2015. júl.
> 28.,
> > > K,
> > > > > 20:19):
> > > > >
> > > > > > Hey,
> > > > > >
> > > > > > I am sorry that you feel bad about this, I only did not add a
> test
> > > case
> > > > > > for FLINK-2419 because I am adding a test in my upcoming PR which
> > > > > > verified the behaviour.
> > > > > >
> > > > > > As for FLINK-2423, it is actually very bad that issue is still
> > there.
> > > > You
> > > > > > introduced this in your PR
> > https://github.com/apache/flink/pull/895
> > > > > which
> > > > > > I commented but no one fixed it before merging. As developing a
> > test
> > > > > takes
> > > > > > quite much time here as it is tricky, I wanted to push the fix,
> > which
> > > > was
> > > > > > in fact trivial.
> > > > > >
> > > > > > Regards,
> > > > > > Gyula
> > > > > >
> > > > > > Robert Metzger <[hidden email]> ezt írta (időpont: 2015.
> júl.
> > > > 28.,
> > > > > > K, 20:01):
> > > > > >
> > > > > >> Hi,
> > > > > >>
> > > > > >> I'm a bit unhappy how we were handling
> > > > > >> https://issues.apache.org/jira/browse/FLINK-2419 today.
> > > > > >>
> > > > > >> I raised a concern in the JIRA because the commit for the fix
> > didn't
> > > > > >> contain any tests. Our coding guidelines [1] imply that every
> > > feature
> > > > > >> should have tests. Apparently there were not enough tests for
> the
> > > two
> > > > > bugs
> > > > > >> fixed with commit 78fd2146dd.
> > > > > >>
> > > > > >> Also, Gyula's answer sounds like he is not willing to add tests
> > > right
> > > > > now.
> > > > > >>
> > > > > >> I can not remember if we ever reverted a commit in the Flink
> > > > community,
> > > > > >> but
> > > > > >> in my understanding, this is how ASF projects are doing lazy
> > > consensus
> > > > > for
> > > > > >> commits-without-PR.
> > > > > >> So if there is a disagreement in the associated JIRA, we revert
> > the
> > > > fix
> > > > > >> until there is an agreement.
> > > > > >>
> > > > > >> In this case, I did not immediately revert the commit, because I
> > > would
> > > > > >> like
> > > > > >> to see whether others in the community agree with me.
> > > > > >>
> > > > > >>
> > > > > >> What do you think how we should handle cases like this one in
> the
> > > > > future?
> > > > > >>
> > > > > >> I think its very important for committers and PMC members to be
> a
> > > good
> > > > > >> example when it comes to following our own rules. Otherwise, how
> > can
> > > > we
> > > > > >> ask
> > > > > >> our contributors to adhere to these rules?
> > > > > >>
> > > > > >>
> > > > > >> My suggestion to resolve this situation is the following:
> > > > > >> - Revert commit 78fd2146dd
> > > > > >> - open pull requests for FLINK-2419 and FLINK-2423 (with tests
> of
> > > > > course),
> > > > > >> review and merge them.
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> Best,
> > > > > >> Robert
> > > > > >>
> > > > > >> [1] http://flink.apache.org/coding-guidelines.html
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>

Stephan Ewen

Re: Revert 78fd2146dd until we have consensus for FLINK-2419?

In reply to this post by Gyula Fóra

I think no one here is adamant about reverting anything for the sake of
reverting. Adding the test now is just fine.

The issue was declaring something as fixed and pushing the responsibility
for testing that away. I agree with Robert that this is not the type of
example by which we should lead the community and voluntary contributors.

It was good to have this discussion, for two reasons:

1) Every commit that does not go through a pull request implicitly seeks
its consensus in hind sight. If that consensus is not reached, there needs
to be a way to resolve this. Discussing, and if necessary, reverting.
Here, we exercised this for the first time.

2) It shows that the guidelines are actually not pure cosmetic. Committers
ask among themselves for the same level of quality (and tedious testing) as
we ask from outside contributors.

Both of them are well.

On Tue, Jul 28, 2015 at 10:44 PM, Gyula Fóra <[hidden email]> wrote: