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 <gyula.f...@gmail.com> wrote: > 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 > > > > > >> > > > > > > > > > > > > > > > > > > > > >
