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 > > > > > >
