flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kostas Tzoumas <ktzou...@apache.org>
Subject Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
Date Tue, 28 Jul 2015 19:36:32 GMT
On Tue, Jul 28, 2015 at 9:09 PM, Gyula Fóra <gyula.fora@gmail.com> 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 <ktzoumas@apache.org> 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 <gyula.fora@gmail.com>
> 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 <gyula.fora@gmail.com> 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 <rmetzger@apache.org> 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
> > > >>
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message