I'd suggest to follow a slightly different protocol where timeout is
more explicit. Any committer can say LGTM (using github approve
changes?) and indicate intention to merge in a day or two (timeout may
depend on complexity of changes and number of active PR reviewers). At
that point any reviewer may either explicitly sign-off (also using
github approve changes) or request more modifications. Committer may
also request any reviewer (likely the one who actively reviewed PR or
who was involved with the functionality in the past) to explicitly
sign-off. After indicated timeout, committer may merge the PR if nobody
explicitly requested changes.
Thank you,
Vlad
//On 3/24/17 14:42, Amol Kekre wrote:
Pramod,
That is a good idea. A timeout will help PR be more efficient.
Thks
Amol
E:a...@datatorrent.com | M: 510-449-2606 | Twitter: @*amolhkekre*
www.datatorrent.com
*Join us at Apex Big Data World Mt View
<http://www.apexbigdata.com/mountain-view.html>, April 4, 2017!*
[image: http://www.apexbigdata.com/san-jose-register.html]
<http://www.apexbigdata.com/san-jose-register.html>
On Fri, Mar 24, 2017 at 11:42 AM, Pramod Immaneni <pra...@datatorrent.com>
wrote:
For the PR part with multiple reviewers, I suggest we pick a convention
like the main reviewer has to wait till all the other reviewers say
something like LGTM or a timeout period like 2 days before merging it. This
will remove ambiguity, especially in cases where reviewers come in
later after review has been happening for a while and it is unclear whether
they have started the review and will have more comments or are just are
making singular comments. We, of course, do not want to encourage the
behavior and would prefer interested folks join the review process early
but in reality, these situations happen.
On Fri, Mar 24, 2017 at 8:38 AM, Thomas Weise <t...@apache.org> wrote:
+1
There are also cases where PRs have been under review by multiple people
that are suddenly unilaterally rebased and merged.
Furthermore those that review and merge should follow the contributor
guidelines (or improve them). For example, JIRAs are supposed to be
resolved and marked with the fix version when the PR is merged.
Thomas
On Thu, Mar 23, 2017 at 12:58 PM, Vlad Rozov <v.ro...@datatorrent.com>
wrote:
Lately there were few instances where PR open against apex-core and
apex-malhar were merged just few hours after it being open and JIRA
being
raised without giving chance for other contributors to review and
comment.
I'd suggest that we stop such practice no matter how trivial those
changes
are. This equally applies to documentation. In a rear cases where PR is
urgent (for example one that fixes compilation error), I'd suggest
that a
committer who plans to merge the PR sends an explicit notification to
dev@apex and gives others a reasonable time to respond.
Thank you,
Vlad
