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