Hello, everybody is free to review PRs, it's in general a good way to become more familiar with the codebase and gain "merit" for committership (like code contributions, helping in the ML/Jira, etc.).
Even if a non committer review doesn't lead directly to code merge (you still need a committer), it generally helps to get the PR in good shape and it makes committers' life easier and it makes PR merging more likely. In summary, no need for any specific tag IMO, reviews from non committers are already more than welcome. Best regards, Alessandro On Mon 14 Aug 2023, 14:53 LakeShen, <[email protected]> wrote: > Sorry, the above email format is a little wrong. The current Code reviews > and feedback from the community are a bit slow,we really need ways to > encourage non-Commtier code review,faster Code reviews and feedback enable > others to participate more actively in the community. Maybe the community > could give some contributors permission to tag PR (not everyone, need to > apply), but not PR approve permission, tags could `include > non-committer-review-accepted`, `return-to-discussion` and so on. At the > same time, good Review feedback is also needed. Anyway, I think we need a > way to encourage non-committer to code reviews and positive content > feedback. Best, LakeShen > > LakeShen <[email protected]> 于2023年8月14日周一 20:40写道： > > > Hi hongyu guo, The current Code reviews and feedback from the community > > are a bit slow,we really need ways to encourage non-Commtier code > review,faster > > Code reviews and feedback enable others to participate more actively in > the > > community. Maybe the community could give some contributors permission to > > tag PR (not everyone, need to apply), but not PR approve permission, tags > > could `include non-committer-review-accepted`, `return-to-discussion` and > > so on. At the same time, good Review feedback is also needed. Anyway, I > > think we need a way to encourage non-committer to code reviews and > > positive content feedback. Best, LakeShen > > > > hongyu guo <[email protected]> 于2023年8月14日周一 19:25写道： > > > >> 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'). > >> > >> > >> Best, > >> Hongyu Guo > >> > >> 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 > >> > < > 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 > >> > <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 > >> > > >>>>> > >> > > >>>> > >> > > >>> > >> > > > >> > > > > > >
