Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
I'm probably lacking a bit of context, but by reading your conversation at JIRA it seems to me that commit https://github.com/apache/flink/commit/78fd2146dd00da1130910d9f23f09e2504854ef7 does not contain a test, and Robert is asking for a test which means that we do not have consensus. If this was a pull request and not a commit it would not be merged until consensus was reached, so my opinion is that the same should happen now: the commit should be reverted, test(s) should be added, and then merged again (or better submitted as a PR). More so as this is deep in the core system code. Kostas On Tue, Jul 28, 2015 at 8:01 PM, Robert Metzger rmetz...@apache.org wrote: 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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
I agree that consensus should be reached in all changes to the system. 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. 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),
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
Re: Revert 78fd2146dd until we have consensus for FLINK-2419?
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
