As a non-committer, I feel extremely glad and excited when my pull request is merged into the main branch. I am not afraid of receiving sharp feedback pointing out my mistakes, but I truly do not want to see no one review my code.
Perhaps introducing a new tag, such as 'non-committer-review-accepted' (or any other suitable name), in GitHub or JIRA could serve as an indication that the pull request is open for review by non-committers. This approach could encourage more non-committers like me to participate in code reviews. Committers can mark this tag when they believe the pull request only introduces a feature in a small part of Calcite or when the pull request is easy to review (e.g., 'add a function to Calcite'). On 2023/07/04 09:34:50 Stamatis Zampetakis wrote: > Here are some stats around PRs merged in the calcite main branch in > the last quarter [2023-04-01, 2023-07-01). The stats are not 100% > accurate to cover reviews done under PRs/jira etc but clearly show > that we are quite far from what we have been discussing here. > > +-----------+---------------------+ > | committer | reviews | > +-----------+---------------------+ > | Julian Hyde <[email protected]> | 28 | > | Benchao Li <[email protected]> | 12 | > | rubenada <[email protected]> | 8 | > | Jiajun <[email protected]> | 8 | > | Stamatis Zampetakis <[email protected]> | 3 | > | Tanner Clary <[email protected]> | 3 > | > | Jacky Lau <[email protected]> | 2 | > | Tanner Clary <[email protected]> | 2 | > | NobiGo <[email protected]> | 2 | > | Feng Zhu <[email protected]> | 1 | > | dssysolyatin <[email protected]> | 1 | > | olivrlee <[email protected]> | 1 | > | Alessandro Solimando <[email protected]> | 1 | > | ILuffZhe <[email protected]> | 1 | > > I would encourage everyone to set some personal goals in terms of > weekly/monthly reviews so that we collectively improve the numbers. > > I will send another update in this thread when I submit the report for > the next quarter Q3 to see if we are making progress or not. > > Best, > Stamatis > > On Thu, Apr 13, 2023 at 6:51 PM Chapuis Bertil <[email protected]> wrote: > > > > +1 > > > > I will likely participate in the effort of reviewing PRs when my schedule > > allows it. > > > > As pointed out in previous comments, some of us have a limited amount of > > time available and few experience with certain areas of the codebase. One > > thing we may experiment with is a shadow review process, similar to what is > > done with shadow program committees at conferences (e.g., S&P [1]). For > > instance, I recently reviewed CALCITE-5160 [2] and asked someone else to > > accept the PR. Frank Zou provided additional feedback, and I learned new > > things along the way. I think such a mechanism would really help > > non-committers and new committers to onboard. > > > > [1] https://www.ieee-security.org/TC/SP2021/shadowpc.html > > [2] https://github.com/apache/calcite/pull/2854 > > > > > > > On 12 Apr 2023, at 11:51, Stamatis Zampetakis <[email protected]> wrote: > > > > > > For a long time this has been one of the main issues of the project > > > and I am happy to see discussions to address this issue. > > > > > > I would like to mention that as a contributor, I am, and always have > > > been very grateful to people reviewing my work. > > > The fact that I became a committer of this project is mainly due to > > > Julian and Vladimir Sitnikov who reviewed and merged many of my PRs. > > > I would definitely like to help and make other contributors feel the > > > same but I cannot really commit to specific volume and deadlines > > > spanning several months. > > > > > > I have the feeling that we don't need the PR manager role. The > > > assignment work can be done by bots (e.g., [1]) if needed and we > > > already have our quarterly stats for reporting purposes. > > > If we want to put a human behind this then it makes more sense for > > > this person to be the release manager; this should be the one nagging > > > people for advancing the work and moving towards the release. > > > > > > Regarding reviews coming from non-committers, I am not sure it's > > > possible to do the assignment in GitHub. It's not a big deal though; > > > for me a simple comment that I am going to review would be sufficient. > > > Alternatively, we could consider adopting an equivalent workflow in > > > JIRA and potentially introducing a new state "IN REVIEW"; don't think > > > it is necessary. > > > No matter the choice we should ensure that we have a trackable way to > > > recognise "non-committer" reviewers but I think both GitHub (e.g., > > > "is:pr reviewed-by:julianhyde is:closed") and JIRA offer the necessary > > > filters; > > > Others projects tend to include such info in the commit message we > > > could also opt for this if we deem necessary. > > > > > > As an immediate action I would encourage everyone willing to help to > > > go to the open PRs on GitHub and either assign some PRs to themselves > > > (in case of committers) or leave a comment about their intention > > > (non-committers). > > > > > > In the meantime we can iterate on this till we reach consensus. > > > > > > Best, > > > Stamatis > > > > > > [1] https://github.com/apps/auto-assign > > > > > > > > > On Wed, Apr 12, 2023 at 10:49 AM Ruben Q L <[email protected]> wrote: > > >> > > >> Hello, > > >> > > >> I understand Julian's frustration. We all know that reviewing PRs is a > > >> recurring problem, and it is not the first time we discuss potential > > >> solutions, see e.g. the discussion a year ago [1] (also started by > > >> Julian) > > >> where several ideas were mentioned: automatic assignment, emulate the RM > > >> process onto the reviewing process (quite similar to the current > > >> proposal), > > >> ...but in the end no change was implemented, and the problem remains. > > >> > > >> I agree that something must be done in order to revert the situation. > > >> > > >> In my opinion, one of the main factors is that the vast majority of PRs > > >> (even the ones that get merged) are never assigned. This lack of assignee > > >> can be seen as if the PR is "no-one's responsibility", so we should try > > >> somehow to assign each PR to someone, and make that person accountable > > >> for > > >> the PR's progression. > > >> I think we could try Julian's idea of having a pool of reviewers and a PR > > >> manager (taken from the pool, rotating this position every month or every > > >> two months). Personally, I would not set hard deadlines (e.g. something > > >> must be done within 3 days), because we are all volunteers and, even if > > >> we > > >> are all trying to do our best here, it may happen that a certain week we > > >> are busy with other personal or professional stuff. In the end, I think > > >> it > > >> should be the PR manager's responsibility to ping the assigned reviewer > > >> if > > >> a PR is not progressing after a reasonable period of time, ask them for > > >> an > > >> update, maybe even involve a different reviewer / re-assign the PR as a > > >> last resource. > > >> > > >> Of course, it must remain clear that, even if we implement this approach, > > >> anyone is still free (and encouraged) to participate in any PR review. > > >> Even > > >> if someone is not the assigned reviewer, they can chime in and contribute > > >> to the review nevertheless. > > >> Also, I think another sensible rule would be: if someone from the > > >> reviewers > > >> pool submits a PR, the PR manager will need to assign it to a different > > >> person. > > >> > > >> One last comment, I have the impression that with this initiative we > > >> would > > >> be moving towards a "better done than perfect" approach. Calcite is a > > >> vast > > >> project, with many different modules, and it could happen (it *will* > > >> happen) that a certain reviewer gets assigned a PR concerning a part of > > >> the > > >> project that they are not familiar with. Of course, one way of becoming > > >> (progressively) familiar with unknown parts of the project is by > > >> reviewing > > >> this type of PRs, but that takes time. I guess it would be possible for > > >> the > > >> assignee to try to ping and involve other reviewers with more experience > > >> in > > >> that area, but at the end of the day, it would be the assignee's > > >> responsibility to review and merge some piece of code that might be a bit > > >> obscure to them. This might lead to suboptimal or even incorrect code > > >> being > > >> inadvertently merged into the main branch. This is not a catastrophe (it > > >> can already happen with the current approach), and we will detect and > > >> correct these mistakes; I'm just mentioning that they might become a bit > > >> more frequent with the proposed approach (and we should all face them > > >> with > > >> a constructive and positive attitude). In any case, I have the impression > > >> that with the new idea the pros outweigh the cons, so we could give it a > > >> try. > > >> > > >> Best, > > >> Ruben > > >> > > >> [1] https://lists.apache.org/thread/30pf1o0vlcn7y3bhlcht1wdmvmxyvghn > > >> > > >> > > >> > > >> On Wed, Apr 12, 2023 at 3:13 AM Chunwei Lei <[email protected]> wrote: > > >> > > >>> Thanks Julian for sharing the proposal. I am +1 for it. I have been > > >>> busy in > > >>> the past few months, so I have only had a quick look at the new JIRA. > > >>> However, I will have more time in the coming months, and I would be more > > >>> than happy to review any pull requests. > > >>> > > >>> > > >>> > > >>> Best, > > >>> Chunwei > > >>> > > >>> > > >>> On Tue, Apr 11, 2023 at 10:22 PM Jiajun Xie <[email protected]> > > >>> wrote: > > >>> > > >>>> Thank Julian for your idea. > > >>>> Your plan helps to motivate new contributors. > > >>>> > > >>>> “If there is no response to my PR, > > >>>> I will be disappointed or even give up on continuing to contribute.” > > >>>> > > >>>> I hope that every contributor will be encouraged, > > >>>> and I also hope that the Calcite community will become stronger and > > >>>> stronger. > > >>>> > > >>>> +1, I am willing to join the pool of reviews. > > >>>> > > >>>> On Tue, 11 Apr 2023 at 13:20, Benchao Li <[email protected]> wrote: > > >>>> > > >>>>> Thanks Julian for starting the discussion! > > >>>>> > > >>>>> I'm spending my spare time to contribute to Calcite, usually at > > >>> weekends, > > >>>>> and sometimes in the break of weekdays, hence my time would be limited > > >>>>> because the spare time may varies. Review work is not that simple for > > >>> me > > >>>>> because Calcite has many complicated components and evolves many years > > >>>>> which means we need track a lot of background. I'm still learning some > > >>>> part > > >>>>> while doing the review work. > > >>>>> > > >>>>> The complexity of PRs varies a lot, simple ones would be easier to get > > >>> in > > >>>>> because it only cost me minutes to hours to review. But the complex > > >>> ones, > > >>>>> usually I need to spend more time to understand the background, new > > >>>> design, > > >>>>> the effect to the whole project, and the future direction we want to > > >>>> take. > > >>>>> These kinds of PRs may be preempted by small ones, and finally do not > > >>>>> getting reviewed for months, there is a example[1] which I would say > > >>>> sorry > > >>>>> to the author that I still do not manage to give it a review till > > >>> today. > > >>>>> > > >>>>> Any way to improve current status would be grateful. However, if the > > >>>>> proposal from Julian may require more sustainable time, I'm not sure > > >>>>> if > > >>>> it > > >>>>> is suitable for guys like me who only devotes limited spare time to > > >>>>> Calcite. Hence I'm +0 for this proposal. Of course, I would be happy > > >>>>> to > > >>>>> participate in the schema if we finally decide to do it. > > >>>>> > > >>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5413 > > >>>>> > > >>>>> Charles Givre <[email protected]> 于2023年4月11日周二 12:43写道： > > >>>>> > > >>>>>> I for one would very much like to help with reviews. I don't have a > > >>>> lot > > >>>>>> of time this month, but next month should have more time. > > >>>>>> Best, > > >>>>>> -- C > > >>>>>> > > >>>>>>> On Apr 10, 2023, at 10:56 PM, Dan Zou <[email protected]> wrote: > > >>>>>>> > > >>>>>>> +1, thanks Julian for proposing this. From my observation, there > > >>> are > > >>>>>> many pending PRs in Calcite and only a few active committers, this > > >>>> puts a > > >>>>>> lot of pressure on these committers. For example Julian have reviewed > > >>>> 34 > > >>>>> PR > > >>>>>> in 2023 Q1, it is an unimaginable number. I am very supportive of > > >>>>> achieving > > >>>>>> a mechanism to improve the review efficiency of PRs, and also I would > > >>>>> like > > >>>>>> to make contribution in reviewing PRs. > > >>>>>>> > > >>>>>>> Best, > > >>>>>>> Dan Zou > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>>> 2023年4月11日 01:56，Julian Hyde <[email protected]> 写道： > > >>>>>>>> > > >>>>>>>> I don't enjoy reviewing and merging PRs. And every time I do, I > > >>> feel > > >>>>>>>> like a sucker, because there are over a few dozen committers who > > >>> are > > >>>>>>>> enjoying the project and not doing the work. (There is a small > > >>> group > > >>>>>>>> of committers who regularly review and merge PRs. I don't know how > > >>>>>>>> they feel about the task, but I am immensely grateful.) > > >>>>>>>> > > >>>>>>>> I think I would review more PRs if I saw others doing the same. > > >>>>>>>> > > >>>>>>>> Can we figure out a fairer way to distribute the load? For release > > >>>>>>>> managers (approximately the same amount of work, but compressed > > >>>> into a > > >>>>>>>> few hours or days) we have successfully run a rota for several > > >>>> years. > > >>>>>>>> Could we do something similar with PRs? > > >>>>>>>> > > >>>>>>>> I propose the following. For each calendar month, there is a PR > > >>>>>>>> manager and 6 - 8 reviewers. The PR manager does not review PRs, > > >>> but > > >>>>>>>> assigns them to reviewers, and politely reminds reviews to keep > > >>> the > > >>>> PR > > >>>>>>>> moving. > > >>>>>>>> > > >>>>>>>> The PR manager's goals are: > > >>>>>>>> * every non-draft PR is reviewed within 3 days of submission, > > >>>>>>>> * every PR is merged within 3 days of being done; > > >>>>>>>> * rotate duties so that no reviewer is asked to review more than 4 > > >>>>>>>> PRs per month; > > >>>>>>>> * email a report at the end of the month; > > >>>>>>>> * work down the backlog of historic PRs if it's a slow month. > > >>>>>>>> > > >>>>>>>> The PR manager rotates every month. The reviewers can rotate if > > >>> they > > >>>>>>>> wish, but I suspect most will stay in the pool for several months, > > >>>>>>>> because the reviewing load is not very heavy, and because they see > > >>>>>>>> others doing the work. > > >>>>>>>> > > >>>>>>>> Other notes: > > >>>>>>>> * Non-committers would be welcome to join the pool of reviews (and > > >>>>>>>> that would be a good way to earn the committer bit) and a > > >>> committer > > >>>>>>>> could merge when the PR is approved. > > >>>>>>>> * If committers join the pool, that's a good way to earn PMC > > >>>>> membership. > > >>>>>>>> * Committers who are not in the pool are welcome to review PRs and > > >>>>>>>> assign PRs to themselves (but expect to be nagged by the PR > > >>> manager > > >>>> if > > >>>>>>>> you don't review in a timely manner). > > >>>>>>>> > > >>>>>>>> What do you think? Would you join this scheme if we introduced it? > > >>>> If > > >>>>>>>> you agree please +1; also happy to see revisions to this > > >>> suggestion > > >>>> or > > >>>>>>>> other ideas to share the work. > > >>>>>>>> > > >>>>>>>> Julian > > >>>>>>> > > >>>>>> > > >>>>>> > > >>>>> > > >>>>> -- > > >>>>> > > >>>>> Best, > > >>>>> Benchao Li > > >>>>> > > >>>> > > >>> > > >
