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
