You never really know whether you have thought of everything. Even if the PR has added tests, and the tests pass, have they tested everything that needs to be tested? If there is an architectural change (and there almost always is some architectural impact, even if it is just doing “more of the same” in an architectural area that is getting a bit creaky) how much burden do we put on the contributor to solve it?
The best you can do is make your best judgment call. I promise never to criticize someone who does what they think best, and I’m sure the other committers feel that way. That said, if you have carried out an initial review and you think a second opinion would help, just ask for it. And by the way, as a project we have never decided whether we were commit-then-review (CTR) or review-then-commit (RTC). The policy has been “do what you think best”, and in my opinion that policy is working. If you are a committer and you think your code doesn’t need review, just go ahead and commit it. I often do this, whereas most other committers usually seem to ask for review (this week I have reviewed PRs from Maryann and Minji, who are both committers), but I am not in a privileged position. Just use your discretion, and you won’t be blamed for it. Julian > On Aug 24, 2017, at 1:54 PM, Josh Elser <[email protected]> wrote: > > Sorry I didn't respond sooner (treading water at $dayjob to stay afloat). > This is a good conversation that we should have as a community. > > I have to echo Michael's assessment of feeling "uncomfortable" reviewing many > contributions due to lack of experience. I recognize that this is a bit of a > fallacy, so, sorry I haven't been able to step up more. > > Abstractly, figuring out the right balance for trust in new contributions > would help remove such a worry. For example, how much trust do I put in a > contribution I might not fully understand if the tests pass? > > If we can get there, at least the only show-stopper is my free time ;) > > On 8/24/17 3:40 PM, Julian Hyde wrote: >> Thanks for your support, Michael. >> Not a peep from the rest of you. I suppose there’s no incentive to speak up >> while the free lunch keeps on coming. >> What should I do to get some assistance from the many people that benefit >> from this being a healthy community? Do I have to threaten to go on strike? >> Julian >>> On Aug 22, 2017, at 8:01 AM, Michael Mior <[email protected]> wrote: >>> >>> I agree this is an important problem to solve. The biggest barrier for me >>> is that I'm not familiar with most of the Calcite code base and I don't >>> really have the time to invest right now in order to become familiar to the >>> point I'd be comfortable reviewing the majority of PRs. I do have >>> notifications of PRs enabled for the repo and will try to make an effort to >>> review when I feel qualified. >>> >>> -- >>> Michael Mior >>> [email protected] <mailto:[email protected]> <mailto:[email protected] >>> <mailto:[email protected]>> >>> >>> 2017-08-21 15:33 GMT-04:00 Julian Hyde <[email protected] >>> <mailto:[email protected]> <mailto:[email protected] >>> <mailto:[email protected]>>>: >>> >>>> I was on vacation last week. Before I went on vacation I sent an email to >>>> this list[0] asking the community of committers to stay on top of pull >>>> requests. My rationale, not explicitly stated in the email but hopefully >>>> clear to everyone, is that we need to respond to contributors in a timely >>>> fashion, otherwise they will not join the community. “Timely” means a day >>>> or two, maximum. >>>> >>>> Since I went on vacation 8 days ago, an existing PR was committed, but 10 >>>> PRs have been created[1], and only one of them received feedback [2]. >>>> (Thanks, Jesus, for the commit and the review.) Several JIRA cases have >>>> been logged, also. >>>> >>>> I don’t think this is an acceptable amount of feedback to sustain the >>>> community. >>>> >>>> The Calcite community is successful and growing, but it takes work to keep >>>> it going. I’m tired of being the main person who does that work. Committers >>>> and PMC members, you all benefit from the community. You (rightly) expect >>>> that your own PRs are reviewed and committed in a timely manner. How can I >>>> encourage you to take a greater share of the work? Do I need to take more >>>> vacation? >>>> >>>> Julian >>>> >>>> [0] http://mail-archives.apache.org/mod_mbox/calcite-dev/ >>>> <http://mail-archives.apache.org/mod_mbox/calcite-dev/> >>>> 201708.mbox/%3CFF047CF1-7B7D-4F4B-BB05-764BB7F9E70D%40apache.org >>>> <http://40apache.org/>%3E < >>>> http://mail-archives.apache.org/mod_mbox/calcite-dev/ >>>> <http://mail-archives.apache.org/mod_mbox/calcite-dev/> >>>> <http://mail-archives.apache.org/mod_mbox/calcite-dev/ >>>> <http://mail-archives.apache.org/mod_mbox/calcite-dev/>> >>>> 201708.mbox/%[email protected] >>>> <mailto:[email protected]> >>>> <mailto:[email protected] >>>> <mailto:[email protected]>>%3E> >>>> >>>> [1] https://github.com/apache/calcite/pulls >>>> <https://github.com/apache/calcite/pulls> >>>> <https://github.com/apache/calcite/pulls >>>> <https://github.com/apache/calcite/pulls>> <https://github.com/apache/ >>>> <https://github.com/apache/> <https://github.com/apache/ >>>> <https://github.com/apache/>> >>>> calcite/pulls> >>>> >>>> [2] https://github.com/apache/calcite/pull/523#pullrequestreview-57275412 >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412> >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412 >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412>> >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412 >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412> >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412 >>>> <https://github.com/apache/calcite/pull/523#pullrequestreview-57275412>>>
