I'd echo Wes's broader point and also potentially encourage downstream projects to run CI against the master branch for catching breakages.
I also think the initial set of recommendations do make sense for some sort of changes (when code does break APIs so more members can weigh in). Also formalizing experimental APIs vs stable APIs and having different policies could make sense. Cheers, Micah On Fri, Jan 29, 2021 at 2:47 PM Wes McKinney <wesmck...@gmail.com> wrote: > 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 > > > > > > > > > > > > > > > > > > >
