When it comes to downstream projects, it may make sense to implement some integration tests that can be triggered via Crossbow if you aren't sure whether a change will cause breakage.
On Fri, Jan 29, 2021 at 1:25 PM Andrew Lamb <al...@influxdata.com> wrote: > > 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 > > > > > > > > > > > > > >
