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 > > > >> > > > > > > > > > >
