Re: [DISCUSS] Code reviews
Thanks for your effort, Stamatis. I totally agree that breaking and other significant changes should be reviewed and receive +1 before committing. Regarding the minimum wait time to commit, I think 72 hours might be too long. Maybe 24-48 hours is enough. Best, Chunwei On Sun, Mar 15, 2020 at 1:07 AM Enrico Olivelli wrote: > Hi, > I see that Calcite is a core and mission critical component for many > applications and libraries, in many core libraries they follow > strictly Review-Than-Commit and this rule is very useful. > > There is no hurry in committing a fix or a new feature, we are a great > opensource project, without time bounds and no need for "hotfixes". > > I see that also new committers feel better with RTC because when it is > the first time you commit to such an important project like Calcite > anyone is really scared to commit > some regression that is not caught by tests ! > > Trivial patches can go without review, but most of the time it is not > worth to hurry, > I totally suggest to at least add the rule that no-one can commit > patches from himself. > It is not a pain to gently ask "please help merging this patch" and > wait for a fellow committer to push the change to master branch. > > Just my 2cents > Enrico > > Il giorno sab 14 mar 2020 alle ore 17:19 Michael Mior > ha scritto: > > > > Thanks for raising the discussion Stamatis! I agree that breaking and > > other significant changes should be reviewed before committing. I'm > > hesitant about saying that *all* issues should be open for 72h before > > committing. Sometimes I'll come up with a small bug fix or enhancement > > that I'd just like to get out and forcing that to happen on separate > > days would significantly slow things down. > > -- > > Michael Mior > > mm...@apache.org > > > > Le sam. 14 mars 2020 à 12:13, Stamatis Zampetakis a > écrit : > > > > > > Hi all, > > > > > > In the recent discussion about the quality of the commit messages [1] > it > > > was brought up the question of having a specific process for code > reviews. > > > I do believe that this is an important subject so I decided to start a > > > separate thread around this topic. > > > > > > Some people lean towards a commit then review (CTR) approach while > others > > > seem to prefer more the review then commit (RTC), which basically > means at > > > least one +1 vote before pushing to master. > > > > > > Personally, I don't this it is necessary to go strictly for one or the > > > other. As a project we have rather high standards on who do we invite > to be > > > a committer so we do put a lot of trust on our members. That is to say > that > > > the committer should be able to judge when it is appropriate to commit > > > directly and when it is necessary to wait for a review. > > > > > > As a rule of thumb if a change does not require a JIRA case then it > does > > > not require a review either. Typical examples would be: typos, code > style, > > > fixing warnings, adding/skipping tests, small doc and site > improvements, > > > announcements, etc. > > > > > > For those changes that do justify a JIRA, we can possibly identify a > few > > > that fall either to CTR or RTC. Nevertheless, I think it would be nice > to > > > have a minimum wait time (72h?) from the moment that a JIRA case is > opened > > > before committing the change to master. This gives plenty of time for > > > people to react and declare the intention to review the change before > > > merging. > > > > > > From my perspective any change that breaks backward compatibility > should > > > follow RTC and receive at least one +1 vote from another committer > before > > > being merged to the master. Obviously, not every change in a public > class > > > should be considered as a breaking change. However, changing the > behaviour > > > or the API of classes/interfaces such as RelOptRule, RelOptPlanner, > > > RelNode, RelBuilder, RexBuilder, RexSimplify, Schema, Table, SqlParser, > > > RelToSqlConverter, SqlToRelConverter, SqlValidator and their main > > > extensions/implementations can have potentially a big impact on > clients so > > > it would be better if we wait for a +1 before merging. > > > > > > For bug fixes that do not break compatibility and do not introduce big > > > changes in the code base I think it is fine to commit without strictly > > > waiting for a review. Bug fixes should (almost) always be accompanied > with > > > a test case that reproduces the problem. > > > > > > In terms of evolutions, if the change is localized and small (e.g., new > > > method in RelBuilder, new SQL function, etc.) I think it is safe to use > > > CTR. In some cases, if there is no review and to be on the safe side, > it > > > might make sense to mark the new API as experimental. > > > > > > Needless to say that for big evolutions the code should receive at > least > > > one positive vote. For the latter, I think we are doing pretty well so > far > > > since such kind of evolutions usually pass
Re: [DISCUSS] Code reviews
Hi, I see that Calcite is a core and mission critical component for many applications and libraries, in many core libraries they follow strictly Review-Than-Commit and this rule is very useful. There is no hurry in committing a fix or a new feature, we are a great opensource project, without time bounds and no need for "hotfixes". I see that also new committers feel better with RTC because when it is the first time you commit to such an important project like Calcite anyone is really scared to commit some regression that is not caught by tests ! Trivial patches can go without review, but most of the time it is not worth to hurry, I totally suggest to at least add the rule that no-one can commit patches from himself. It is not a pain to gently ask "please help merging this patch" and wait for a fellow committer to push the change to master branch. Just my 2cents Enrico Il giorno sab 14 mar 2020 alle ore 17:19 Michael Mior ha scritto: > > Thanks for raising the discussion Stamatis! I agree that breaking and > other significant changes should be reviewed before committing. I'm > hesitant about saying that *all* issues should be open for 72h before > committing. Sometimes I'll come up with a small bug fix or enhancement > that I'd just like to get out and forcing that to happen on separate > days would significantly slow things down. > -- > Michael Mior > mm...@apache.org > > Le sam. 14 mars 2020 à 12:13, Stamatis Zampetakis a écrit > : > > > > Hi all, > > > > In the recent discussion about the quality of the commit messages [1] it > > was brought up the question of having a specific process for code reviews. > > I do believe that this is an important subject so I decided to start a > > separate thread around this topic. > > > > Some people lean towards a commit then review (CTR) approach while others > > seem to prefer more the review then commit (RTC), which basically means at > > least one +1 vote before pushing to master. > > > > Personally, I don't this it is necessary to go strictly for one or the > > other. As a project we have rather high standards on who do we invite to be > > a committer so we do put a lot of trust on our members. That is to say that > > the committer should be able to judge when it is appropriate to commit > > directly and when it is necessary to wait for a review. > > > > As a rule of thumb if a change does not require a JIRA case then it does > > not require a review either. Typical examples would be: typos, code style, > > fixing warnings, adding/skipping tests, small doc and site improvements, > > announcements, etc. > > > > For those changes that do justify a JIRA, we can possibly identify a few > > that fall either to CTR or RTC. Nevertheless, I think it would be nice to > > have a minimum wait time (72h?) from the moment that a JIRA case is opened > > before committing the change to master. This gives plenty of time for > > people to react and declare the intention to review the change before > > merging. > > > > From my perspective any change that breaks backward compatibility should > > follow RTC and receive at least one +1 vote from another committer before > > being merged to the master. Obviously, not every change in a public class > > should be considered as a breaking change. However, changing the behaviour > > or the API of classes/interfaces such as RelOptRule, RelOptPlanner, > > RelNode, RelBuilder, RexBuilder, RexSimplify, Schema, Table, SqlParser, > > RelToSqlConverter, SqlToRelConverter, SqlValidator and their main > > extensions/implementations can have potentially a big impact on clients so > > it would be better if we wait for a +1 before merging. > > > > For bug fixes that do not break compatibility and do not introduce big > > changes in the code base I think it is fine to commit without strictly > > waiting for a review. Bug fixes should (almost) always be accompanied with > > a test case that reproduces the problem. > > > > In terms of evolutions, if the change is localized and small (e.g., new > > method in RelBuilder, new SQL function, etc.) I think it is safe to use > > CTR. In some cases, if there is no review and to be on the safe side, it > > might make sense to mark the new API as experimental. > > > > Needless to say that for big evolutions the code should receive at least > > one positive vote. For the latter, I think we are doing pretty well so far > > since such kind of evolutions usually pass first from a discussion in the > > dev list and I think it is a good idea to continue to do so. > > > > Finally regarding the content and quality of a review there are many nice > > articles out there so I am not going to outline a big list with "Dos" and > > "Donts" here. I will just mention the three filters approach [2] which I > > think is a helpful tool to keep in mind when doing reviews. > > > > The above describe more or less the way I operate my self so it is normal > > if people feel and want to operate differently. I just wanted to
Re: [DISCUSS] Code reviews
Thanks for raising the discussion Stamatis! I agree that breaking and other significant changes should be reviewed before committing. I'm hesitant about saying that *all* issues should be open for 72h before committing. Sometimes I'll come up with a small bug fix or enhancement that I'd just like to get out and forcing that to happen on separate days would significantly slow things down. -- Michael Mior mm...@apache.org Le sam. 14 mars 2020 à 12:13, Stamatis Zampetakis a écrit : > > Hi all, > > In the recent discussion about the quality of the commit messages [1] it > was brought up the question of having a specific process for code reviews. > I do believe that this is an important subject so I decided to start a > separate thread around this topic. > > Some people lean towards a commit then review (CTR) approach while others > seem to prefer more the review then commit (RTC), which basically means at > least one +1 vote before pushing to master. > > Personally, I don't this it is necessary to go strictly for one or the > other. As a project we have rather high standards on who do we invite to be > a committer so we do put a lot of trust on our members. That is to say that > the committer should be able to judge when it is appropriate to commit > directly and when it is necessary to wait for a review. > > As a rule of thumb if a change does not require a JIRA case then it does > not require a review either. Typical examples would be: typos, code style, > fixing warnings, adding/skipping tests, small doc and site improvements, > announcements, etc. > > For those changes that do justify a JIRA, we can possibly identify a few > that fall either to CTR or RTC. Nevertheless, I think it would be nice to > have a minimum wait time (72h?) from the moment that a JIRA case is opened > before committing the change to master. This gives plenty of time for > people to react and declare the intention to review the change before > merging. > > From my perspective any change that breaks backward compatibility should > follow RTC and receive at least one +1 vote from another committer before > being merged to the master. Obviously, not every change in a public class > should be considered as a breaking change. However, changing the behaviour > or the API of classes/interfaces such as RelOptRule, RelOptPlanner, > RelNode, RelBuilder, RexBuilder, RexSimplify, Schema, Table, SqlParser, > RelToSqlConverter, SqlToRelConverter, SqlValidator and their main > extensions/implementations can have potentially a big impact on clients so > it would be better if we wait for a +1 before merging. > > For bug fixes that do not break compatibility and do not introduce big > changes in the code base I think it is fine to commit without strictly > waiting for a review. Bug fixes should (almost) always be accompanied with > a test case that reproduces the problem. > > In terms of evolutions, if the change is localized and small (e.g., new > method in RelBuilder, new SQL function, etc.) I think it is safe to use > CTR. In some cases, if there is no review and to be on the safe side, it > might make sense to mark the new API as experimental. > > Needless to say that for big evolutions the code should receive at least > one positive vote. For the latter, I think we are doing pretty well so far > since such kind of evolutions usually pass first from a discussion in the > dev list and I think it is a good idea to continue to do so. > > Finally regarding the content and quality of a review there are many nice > articles out there so I am not going to outline a big list with "Dos" and > "Donts" here. I will just mention the three filters approach [2] which I > think is a helpful tool to keep in mind when doing reviews. > > The above describe more or less the way I operate my self so it is normal > if people feel and want to operate differently. I just wanted to share my > thoughts on the subject. > > Best, > Stamatis > > [1] > https://lists.apache.org/thread.html/r82f50cbea40d5e003fabd41b14743c417d4c8c31f02f9f51c6c9da26%40%3Cdev.calcite.apache.org%3E > [2] > https://phauer.com/2018/code-review-guidelines/#three-filters-for-feedback
[DISCUSS] Code reviews
Hi all, In the recent discussion about the quality of the commit messages [1] it was brought up the question of having a specific process for code reviews. I do believe that this is an important subject so I decided to start a separate thread around this topic. Some people lean towards a commit then review (CTR) approach while others seem to prefer more the review then commit (RTC), which basically means at least one +1 vote before pushing to master. Personally, I don't this it is necessary to go strictly for one or the other. As a project we have rather high standards on who do we invite to be a committer so we do put a lot of trust on our members. That is to say that the committer should be able to judge when it is appropriate to commit directly and when it is necessary to wait for a review. As a rule of thumb if a change does not require a JIRA case then it does not require a review either. Typical examples would be: typos, code style, fixing warnings, adding/skipping tests, small doc and site improvements, announcements, etc. For those changes that do justify a JIRA, we can possibly identify a few that fall either to CTR or RTC. Nevertheless, I think it would be nice to have a minimum wait time (72h?) from the moment that a JIRA case is opened before committing the change to master. This gives plenty of time for people to react and declare the intention to review the change before merging. >From my perspective any change that breaks backward compatibility should follow RTC and receive at least one +1 vote from another committer before being merged to the master. Obviously, not every change in a public class should be considered as a breaking change. However, changing the behaviour or the API of classes/interfaces such as RelOptRule, RelOptPlanner, RelNode, RelBuilder, RexBuilder, RexSimplify, Schema, Table, SqlParser, RelToSqlConverter, SqlToRelConverter, SqlValidator and their main extensions/implementations can have potentially a big impact on clients so it would be better if we wait for a +1 before merging. For bug fixes that do not break compatibility and do not introduce big changes in the code base I think it is fine to commit without strictly waiting for a review. Bug fixes should (almost) always be accompanied with a test case that reproduces the problem. In terms of evolutions, if the change is localized and small (e.g., new method in RelBuilder, new SQL function, etc.) I think it is safe to use CTR. In some cases, if there is no review and to be on the safe side, it might make sense to mark the new API as experimental. Needless to say that for big evolutions the code should receive at least one positive vote. For the latter, I think we are doing pretty well so far since such kind of evolutions usually pass first from a discussion in the dev list and I think it is a good idea to continue to do so. Finally regarding the content and quality of a review there are many nice articles out there so I am not going to outline a big list with "Dos" and "Donts" here. I will just mention the three filters approach [2] which I think is a helpful tool to keep in mind when doing reviews. The above describe more or less the way I operate my self so it is normal if people feel and want to operate differently. I just wanted to share my thoughts on the subject. Best, Stamatis [1] https://lists.apache.org/thread.html/r82f50cbea40d5e003fabd41b14743c417d4c8c31f02f9f51c6c9da26%40%3Cdev.calcite.apache.org%3E [2] https://phauer.com/2018/code-review-guidelines/#three-filters-for-feedback
