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 <ktzou...@apache.org> ezt írta (időpont: 2015. júl. 28., K, 21:36): > On Tue, Jul 28, 2015 at 9:09 PM, Gyula Fóra <gyula.f...@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 <ktzou...@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.f...@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.f...@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 <rmetz...@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 > > > > >> > > > > > > > > > > > > > > >
