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 <eolive...@gmail.com> 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 > <mm...@apache.org> 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 <zabe...@gmail.com> 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 >
