Micah, it is a great question. I often find myself thinking "is this PR ok to merge" and going by gut feel of if it has sufficient review and consensus. I think most of the time these decisions are sound, but at least once it was not (that particular instance I think has been since sorted satisfactorily). This experience made me realize were I to be asked "why did you merge this PR and not that other PR" I would have no objective criteria to point to
I also don't want to strangle the Rust implementation with an overly burdensome process so striking the right balance will be key. Andrew On Fri, Jan 29, 2021 at 2:19 PM Andy Grove <andygrov...@gmail.com> wrote: > One of the challenges that we face in the Rust implementation is that some > parts of the project, especially DataFusion, are still moving fast, with > frequent breaking changes, and because there are now multiple projects that > depend on it, we need to be thoughtful about the impact of these changes on > other projects. At least, that's my perspective on this. > > On Fri, Jan 29, 2021 at 10:32 AM Micah Kornfield <emkornfi...@gmail.com> > wrote: > > > Just curious what is driving the formalization of a policy? Have Rust > > contributions been having issues? > > > > We don't have a 2 reviewer requirement for any of the other languages as > > far as I know. And committers generally use their judgement if a second > > reviewer is necessary. > > > > Same question about leaving a PR open for additional comment. I would > > think this would also be somewhat dependent on the scope of the change, > but > > hopefully most reasonably sized PRs would not be too contentious > (providing > > the committer reviewing feels comfortable in that area of the code base). > > > > Thanks, > > Micah > > > > On Fri, Jan 29, 2021 at 8:54 AM Jorge Cardoso Leitão < > > jorgecarlei...@gmail.com> wrote: > > > > > Hi, > > > > > > Thanks a lot for writing. I like it very much. > > > > > > Some follow ups / clarifications: > > > > > > Do we differentiate between who PRed? I.e. if it is a committer, do > they > > > count for the approval or in that case do we need 2 approvals? I am > more > > > favourable to require a second approval as usual, with the idea that I > > > prefer to have more enrollment by the broader community to the review > > > process. OTOH, this may be too much of a burden if committers are the > > only > > > ones reviewing PRs (i.e. it requires 3 committers: author + 2). > > > > > > Do we treat backward incompatible changes differently? For example, I > > > hardly merge API changes to DataFusion without Andy's approval of the > PR. > > > My reasoning is that there may be a larger design/architectural > decision > > > for the current API (e.g. matches spark's, enables distributed engines) > > > that I am unaware of when reviewing a specific / narrow PR and I very > > much > > > appreciate Andy's check that the bigger picture remains consistent. I > do > > > not have an opinion here, I just wanted to express this implicit rule > > that > > > I use. > > > > > > Best, > > > Jorge > > > > > > > > > On Fri, Jan 29, 2021 at 12:39 PM Andrew Lamb <al...@influxdata.com> > > wrote: > > > > > > > One of the items that we discussed at Wednesday's Rust sync was "what > > is > > > > the criteria to merge a Rust PR". There was no conclusion at the > > meeting, > > > > but there was a proposal which we would like to discuss on the > mailing > > > > list. > > > > > > > > *Goal*: Keep Arrow Rust PRs flowing in a timely fashion, thereby > > keeping > > > > velocity high and encouraging additional contributions, while also > > > ensuring > > > > that quality is maintained and all contributors have a chance to > weigh > > in > > > > prior to merge. > > > > > > > > *Proposed Guideline:* (mostly a formalization of what I see happening > > > > already): > > > > > > > > 1. Have 2 approvals prior to merging a PR, with at least one from a > > > > committer > > > > 2. Have been open for several days to allow interested parties time > to > > > > comment > > > > 3. All comments have been addressed (including honoring requests for > > > > additional time to review) > > > > > > > > Some flexibility in the rules is likely important: there are > different > > > > parts of the code at fairly different levels of maturity, and there > are > > > > some parts of the code (e.g. some parts of the parquet code base that > > > don’t > > > > have a large number of reviewers) > > > > > > > > For small changes such as fixing CI, or formatting, less time / fewer > > > > reviews are ok, as determined by the judgement of the committer. > > > > > > > > Please let us know your thoughts, > > > > Andrew > > > > > > > > > >
