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