Hello, I agree that this is a long-lasting issue in our project. Normally, when I see a lack of reaction in one of my PRs, I check the impacted file(s) history and try to contact directly someone that has worked on it in the past in order to help with the review. But I admit this is an imperfect approach: - On the one hand, there are people (e.g. Julian Hyde) who have massively contributed to many parts of the project, so they will get overwhelmed with requests (and I sincerely apologize for that). - On the other hand, there might be other contributors that know that part of the code, but have never committed anything on it; so even though they do not appear on the history, they could perfectly contribute to the PR review.
I share Zoltan's concerns about "forcing" committers to act as reviewers in parts of the project that they might not know. Wouldn't they feel uncomfortable or insecure if they need to decide about these patches? Should they take responsibility in this situation and do their best (even if it may result in "problematic code" being committed)? Or should they act as an intermediary in this case and dispatch the PR to an appropriate reviewer (or at least ask some advice from them)? How do we find this "appropriate reviewer"? Wouldn't we get into the same situation of overloading the same people as today? Automatic assignment is an interesting idea, although it may be difficult to implement (what goes to whom), and it may not guarantee to solve the problem. To some extent, I think something that can indirectly help on this issue is keeping a stable, predictable and frequent release rhythm. If we release a new version every let's say two months, and we warn with enough anticipation that in the coming weeks a new RC will be produced, people would normally react and "relaunch" their pending PRs so that they can be part of the release. At this point, if we all collectively make an effort to review these PR candidates, we can partially unblock the PR bottleneck. We have seen this in the past. Although this does not address the root cause of the review problem, it can help if we release with a high frequency (as I believe it is our intention), because we would have every two months a period of "PR review marathon" with (hopefully) a lot of volunteers to help reviewing and unblocking the pending PRs to prepare the release. Best, Ruben On Sun, Feb 13, 2022 at 9:18 AM Yanjing Wang <[email protected]> wrote: > Hi Julian, > Sorry for causing much trouble to you. I think no responsiveness will make > contributors disappointed, so I suggest we need give a rough time when the > pr will be reviewed. And we need a periodic report that shows how many prs > have been left out for a long time, then encourage more people including > contributors to participate in the review process. > > Zoltan Haindrich <[email protected]> 于2022年2月11日周五 18:16写道： > > > Hey, > > > > > I think the main problem is still the lack of active > committers/reviewers > > > and not so much the assignment. > > > > Totally agree - I think this is the core of the problem; the > > assign-by-files could somewhat help pulling in the right people - but > will > > not make the job done. > > > > > We could opt for assigning PRs automatically using filename patterns. > > This > > > can be done rather easily and it's already done in various projects > e.g., > > > Hive [1]. > > > > I pioneered to make that happen - but I think it didn't really lifted > off; > > for something like this to work; people should sign on volunteeringly (in > > which I think Calcite > > is stronger). > > I would definetly sign on to review some parts - but now I know: that > even > > with these requests there is no guarantee that I'll take a look... > > One aspect of implementing this was to provide a way to keep an eye on > key > > points of the project (in case of Hive: thrift api; metastore schema) to > > avoid possible issues > > arising from conceptional problems. > > > > Julian> Suppose that six committers volunteer to review and merge up to > > six PRs per quarter > > I think this is an interesting idea but given the broad range of things > we > > have in Calcite; a PR could range from linq4j to say the Geode adapter... > > If you open https://github.com/apache/calcite/pulls how many you feel > > confident enough to give a "merge" (optionally after chanages) or "close > > PR" decision for it? > > > > I think what makes this project great is that we have a lot of people > here > > with strong views and high standards - this is good in a sense that the > > best solution is choosen > > in most cases. > > The presence of the above also puts more weight on committers to accept > > only changes which are up to these high standards. > > > > I'm not sure how many of you have merged in a PR from a contributor which > > contained serious issues - which have surfaced later and caused issues. > > Did you feeled ashamed that you let that patch in - and was not doing > your > > part correctly. Because I did... I know its community effort and > everything > > but afterall it was > > me who pushed the merge button... > > > > I had the opportunity to experience a support process built on a similar > > principle "you just have to carry it" and I have to say that a process > like > > this to work I see the > > following options: > > * we should have people to ask help from - who actually do respond > > ** note that in this case the review and the decision to accept the > change > > would *still* stay with the original owner/etc - so it will not mean much > > improvement beyond > > having 1 more people in the chain who is shouting out for others... > > * accept that sometimes non-optimal solution will be choosen (which are > in > > most cases still valuable) > > > > I think in case of a release its more clear what people are signing up > > for; its documented/etc. > > For reviews its a bit different; with the proposed system: > > * what if you don't find 6 PRs you are confident to be able to review in > a > > quarter? > > * there will be 6 people on this mission at the same time - which could > > further increase the chance that people would not take risks > > > > cheers, > > Zoltan > > > > > > On 2/7/22 10:56 PM, Stamatis Zampetakis wrote: > > > Hello, > > > > > > I think the main problem is still the lack of active > committers/reviewers > > > and not so much the assignment. > > > > > > Nevertheless, things would be a bit better if we had people assigned to > > > PRs. This could certainly help at least offload some work from Julian > and > > > possibly sensibilize more people towards the reviewing process. > > > > > > We could opt for assigning PRs automatically using filename patterns. > > This > > > can be done rather easily and it's already done in various projects > e.g., > > > Hive [1]. > > > > > > What do you think of putting automatic assignment in place for Calcite? > > > Who is willing to put their name in the list? At this point I am not > > > expecting that people in the list will review everything they are > > assigned > > > on but maybe they can help out by pointing to other people or simply > > > setting up the expectations about the PR getting reviewed. > > > > > > Best, > > > Stamatis > > > > > > [1] > > > > > > https://github.com/apache/hive/commit/2bd6a9d63c28e5cb9bcc44115262d565cdc2bb90 > > > > > > > > > On Sat, Feb 5, 2022 at 5:52 PM Jing Zhang <[email protected]> > wrote: > > > > > >> Hi, Julian > > >> There is no doubt that you are the most prolific contributors on the > > >> project. > > >> People often hope to hear professional advice from you because you are > > the > > >> Calcite project authority. > > >> However people may didn't realize that you are suffering from more and > > more > > >> requests. I want to sincerely apologize because I often disturb you > too. > > >> I'm really glad to hear your feelings. > > >> At the same time, I want to thank you because I got many professional > > >> advices from you. The suggestions are very helpful. > > >> > > >> Back to this topic, having an efficient mechanism to merge > > contributors' PR > > >> is very important to the long-term development of open source > projects. > > >> I would like to share my thoughts, hope it helps. > > >> 1. It might helpful to know which members are proficient in which > > modules. > > >> For example, introduce each member's familiar module on the > website[1]. > > >> There may be many requests sent to other members. > > >> 2. For some modules, perhaps only very few members are familiar. (For > > >> example, when it comes to modifying the parser grammar, someone may > > want to > > >> hear from Julian, and when it comes to hints, someone may want to hear > > from > > >> Danny.) Maybe it's just my bias. But if this is the real situation, is > > it > > >> possible to develop more than one members on each module? > > >> 3. It could be helpful to assign reviewers for a new pull request. > > >> > > >> [1] https://calcite.apache.org/community/#project-members > > >> > > >> Best, > > >> Jing Zhang > > >> > > >> Julian Hyde <[email protected]> 于2022年2月4日周五 02:18写道： > > >> > > >>> I make many contributions to this project, in the form of code, > > >>> answering questions, leading design discussions, and clarifying bugs > > >>> and feature requests. I review more changes than any other project > > >>> member. My reward is that I am pestered, daily, with people pleading > > >>> for me to review their changes. > > >>> > > >>> It's moderately annoying for me - I just delete the emails (because I > > >>> have 200 other emails to delete before I can start productive work). > > >>> But it's awful for both the contributors and the project. > > >>> > > >>> Getting PRs reviewed is this project's biggest problem, and has been > > for > > >>> years. > > >>> > > >>> Many of you are team leads, engineering managers, directors of > > >>> engineering. This is a process problem. Solving problems like this is > > >>> what you do. Fix it. > > >>> > > >>> Julian > > >>> > > >> > > > > > >
