[Rust] Proposed PR Merge Guidelines

[Andrew Lamb <al...@influxdata.com>]

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

Re: [Rust] Proposed PR Merge Guidelines

[Andy Grove <an...@gmail.com>]

Thanks for writing this up, Andrew. I think this looks good.

One challenge for me, and I assume I am not alone, is that I generally only
have time at weekends to review non-trivial PRs.

I don't think there is a good solution to this problem but I will comment
on the PRs that I have a particular interest in reviewing to request
additional time to review.

Andy.

On Fri, Jan 29, 2021 at 4:39 AM 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
>

Re: [Rust] Proposed PR Merge Guidelines

[Micah Kornfield <em...@gmail.com>]

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 <we...@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 <an...@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 <
> emkornfield@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 <
> > > > jorgecarleitao@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 <alamb@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
> > > > > >
> > > > >
> > > >
> > >
>

Re: [Rust] Proposed PR Merge Guidelines

[Wes McKinney <we...@gmail.com>]

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 <an...@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 <em...@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 <
> > > jorgecarleitao@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
> > > > >
> > > >
> > >
> >

Re: [Rust] Proposed PR Merge Guidelines

[Andrew Lamb <al...@influxdata.com>]

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 <an...@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 <em...@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 <
> > jorgecarleitao@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
> > > >
> > >
> >
>

Re: [Rust] Proposed PR Merge Guidelines

[Andy Grove <an...@gmail.com>]

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 <em...@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 <
> jorgecarleitao@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
> > >
> >
>

Re: [Rust] Proposed PR Merge Guidelines

[Micah Kornfield <em...@gmail.com>]

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

Re: [Rust] Proposed PR Merge Guidelines

[Jorge Cardoso Leitão <jo...@gmail.com>]

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
>