<div dir="ltr">+1 - for the last time :D<div><br></div><div>Originally I used +1/LGTM to let others look into the PR before integrating it. But I agree - it was impractical. Let's integrate stuff instead...</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 5, 2016 at 11:51 AM, Dan Berindei <span dir="ltr"><<a href="mailto:dan.berindei@gmail.com" target="_blank">dan.berindei@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I would also suggest avoiding +1/LGTM comments. It's enough to have<br>
the author and the integrator responsible in case something goes<br>
wrong, we don't need to "spread the blame" by having multiple +1<br>
comments.<br>
<br>
"LGTM, but" is fine if there's a specific area that you have doubts<br>
about, and you're assigning the PR to someone else. Otherwise, if it<br>
looks good, then we should integrate it without superfluous comments<br>
:)<br>
<br>
Cheers<br>
<span class="HOEnZb"><font color="#888888">Dan<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On Wed, Oct 5, 2016 at 12:37 PM, Sebastian Laskawiec<br>
<<a href="mailto:slaskawi@redhat.com">slaskawi@redhat.com</a>> wrote:<br>
> I agree with both Tristan and Sanne - they key point here is to make<br>
> reviewing PRs a habit. Doing it once a day (or even more often as Tristan<br>
> suggested) is a good place to start in my opinion.<br>
><br>
> The "rebase and merge" can speed the things up but it doesn't solve the main<br>
> problem. We simply need to be more courageous when integrating PRs and avoid<br>
> blocking project's progress.<br>
><br>
> Thanks<br>
> Sebastian<br>
><br>
><br>
><br>
> On Wed, Oct 5, 2016 at 11:21 AM, Sanne Grinovero <<a href="mailto:sanne@infinispan.org">sanne@infinispan.org</a>><br>
> wrote:<br>
>><br>
>> Nice suggestions. I would be careful in actually *not* creating<br>
>> dedicated "pr merge sprints" as during such a sprint one would have<br>
>> more frequent merges, and so making the keep-up-rebasing and conflict<br>
>> resolving even more painful ;)<br>
>><br>
>> I think the only solution is - like Tristan suggests - to make it a<br>
>> discipline to regularly check for PRs to be merged.<br>
>><br>
>> Slightly unrelated, but it might help: did you notice that Github<br>
>> recently evolved their PR merge button?<br>
>> For trivial changes, i.e. if you're confident to not need to re-run<br>
>> the testsuite after a rebase, you can now just press the green button<br>
>> to have it rebase & merge, without creating a merge point.<br>
>><br>
>> I just enabled the feature on the Infinispan repo ;)<br>
>><br>
>> Sanne<br>
>><br>
>><br>
>> On 5 October 2016 at 09:18, Tristan Tarrant <<a href="mailto:ttarrant@redhat.com">ttarrant@redhat.com</a>> wrote:<br>
>> > [Moving to infinispan-dev]<br>
>> ><br>
>> > I agree with all of you.<br>
>> ><br>
>> > It is not respectful towards the developer who has opened the PR to let<br>
>> > it linger for longer than necessary, and also for the developer who has<br>
>> > opened it not to react on the comments (that happens too!)<br>
>> ><br>
>> > Let's set some ground rules for Pull Requests (this should go in the<br>
>> > Contribution guide):<br>
>> ><br>
>> > Rules for everybody<br>
>> > - Each and everyone of the core engineers looks at the PR queue at least<br>
>> > once per day. More frequently if possible, especially if engaging in an<br>
>> > active collaborative review.<br>
>> > - We have more than one project: cachestores, console, integrations,<br>
>> > quickstarts, clients, website...<br>
>> ><br>
>> > Rules for PR owners:<br>
>> > - PRs *must* be kept "applicable" (no conflicts)<br>
>> > - non-trivial PRs *must* have a corresponding Jira and the link to it<br>
>> > *must* be present in the PR description. Viceversa, the Jira's workflow<br>
>> > must be advanced to "Pull Request Sent" and include the link to the PR<br>
>> > - if the PR is for multiple branches and it cherry-picks cleanly to all<br>
>> > of them, no need to open multiple PRs. Use the "Backport" label to<br>
>> > indicate this<br>
>> > - if there is a need for separate PRs for different branches, clearly<br>
>> > indicate the non-master branch it needs to be applied to in the PR title<br>
>> > using the [branch] notation<br>
>> > - Ensure the correct labels are applied when opening a PR or when the<br>
>> > status changes<br>
>> > - The owner *must* react to comments / CI failures / requests for rebase<br>
>> > within 24 hours. Reaction means acknowledgement, not necessarily being<br>
>> > able to fix the issues<br>
>> > - If the owner cannot fix the issues related to comments / CI within a<br>
>> > week, the PR should be closed and reopened when they can be addressed<br>
>> > - While we still have some intermittent CI failures, we're in a<br>
>> > situation where it is quite reliable. Exceptions obviously happen.<br>
>> ><br>
>> > Rules for PR reviewers<br>
>> > - A PR should be acted upon (commented, merged) within 24 hours of its<br>
>> > opening<br>
>> > - A PR can be commented upon even if CI hasn't finished its job<br>
>> > - PRs opened by external developers won't have the appropriate labels<br>
>> > for permission reasons. Add them.<br>
>> > - If the owner has addressed all comments / CI failures, the PR should<br>
>> > be merged within a week<br>
>> > - Some effects of a PR cannot be detected by CI. This for example<br>
>> > includes verifying that docs / PDFs / etc render correctly, that<br>
>> > distribution packages contain the appropriate files, etc. Do your best<br>
>> > to evaluate these.<br>
>> ><br>
>> ><br>
>> > With that being said, the solution for the current situation is to<br>
>> > divide the queue among ourselves and work through it using the sprint<br>
>> > approach (but we need to be careful with stability: "commit storms" can<br>
>> > be detrimental).<br>
>> ><br>
>> > Tristan<br>
>> ><br>
>> > On 05/10/16 09:39, Radim Vansa wrote:<br>
>> >> I think that can be combined: sprint just explicitly prioritizes PRs<br>
>> >> instead of normal development, so you won't be reviewing PRs only when<br>
>> >> "you have a free spot" but "until you can't review anything else". So<br>
>> >> once the PR count hits a threshold, Tristan schedules sprint.<br>
>> >><br>
>> >> R.<br>
>> >><br>
>> >> On 10/05/2016 09:17 AM, Sebastian Laskawiec wrote:<br>
>> >>> That's an interesting idea... but after a month or two, the PR queue<br>
>> >>> will pile up again and we will need another sprint...<br>
>> >>><br>
>> >>> IMO we should have a day-to-day strategy for doing this.<br>
>> >>><br>
>> >>> On Wed, Oct 5, 2016 at 9:07 AM, Gustavo Fernandes <<a href="mailto:gfernand@redhat.com">gfernand@redhat.com</a><br>
>> >>> <mailto:<a href="mailto:gfernand@redhat.com">gfernand@redhat.com</a>>> wrote:<br>
>> >>><br>
>> >>> I propose doing with PRs the same we've been doing to areas that<br>
>> >>> require some care, the so called 'sprints'.<br>
>> >>><br>
>> >>> We are in more than 10, a couple of PRs integrated each and we can<br>
>> >>> get<br>
>> >>> rid of most of the queue.<br>
>> >>><br>
>> >>> Gustavo<br>
>> >>><br>
>> >>> On Wed, Oct 5, 2016 at 7:36 AM, Sebastian Laskawiec<br>
>> >>> <<a href="mailto:slaskawi@redhat.com">slaskawi@redhat.com</a> <mailto:<a href="mailto:slaskawi@redhat.com">slaskawi@redhat.com</a>>> wrote:<br>
>> >>> > Hey guys!<br>
>> >>> ><br>
>> >>> > I'm not sure what do you think about our PR queue but for me it<br>
>> >>> simply<br>
>> >>> > sucks...<br>
>> >>> ><br>
>> >>> > Some of our PRs are sitting there since I remember (like this<br>
>> >>> one [1] - Nov<br>
>> >>> > 2015!!! I guess we should prepare an anniversary cake, November<br>
>> >>> will is<br>
>> >>> > really soon :D) and some of them have more than 150 comments [2]<br>
>> >>> (maybe this<br>
>> >>> > sounds weird since it's my PR, but if something is not good<br>
>> >>> enough after 150<br>
>> >>> > comments, maybe we should throw it away and try to implement it<br>
>> >>> again -<br>
>> >>> > differently?).<br>
>> >>> ><br>
>> >>> > After thinking about this for a little while, I would like to<br>
>> >>> propose one<br>
>> >>> > rule for reviewing PRs - when you have a free spot and you'd<br>
>> >>> like to<br>
>> >>> > integrate a couple of PRs - always start with the oldest and go<br>
>> >>> through all<br>
>> >>> > "Ready for review" PRs.<br>
>> >>> ><br>
>> >>> > I know that integrating some new stuff will be tempting but<br>
>> >>> please don't do<br>
>> >>> > that. Even if the new PR modifies only one small line.. noooo.<br>
>> >>> Go through<br>
>> >>> > some old stuff instead. This way we should revisit old PRs much<br>
>> >>> more<br>
>> >>> > frequently than new ones and this will force us to make some<br>
>> >>> tough decisions<br>
>> >>> > that we avoided for quite some time (e.g. whether or not we<br>
>> >>> should integrate<br>
>> >>> > [1] or [2] - but those are only examples).<br>
>> >>> ><br>
>> >>> > And finally, I think we should have more courage to integrate<br>
>> >>> PRs especially<br>
>> >>> > into master branch... After all, we have all machinery in place<br>
>> >>> - nightly CI<br>
>> >>> > jobs, stress tests, performance tests - what bad can happen? We<br>
>> >>> break the<br>
>> >>> > build? So what? We can always fix it by git revert.<br>
>> >>> ><br>
>> >>> > What do you think? Maybe you have better ideas?<br>
>> >>> ><br>
>> >>> > Thanks<br>
>> >>> > Seb<br>
>> >>> ><br>
>> >>> > [1] <a href="https://github.com/infinispan/infinispan/pull/3860" rel="noreferrer" target="_blank">https://github.com/infinispan/<wbr>infinispan/pull/3860</a><br>
>> >>> <<a href="https://github.com/infinispan/infinispan/pull/3860" rel="noreferrer" target="_blank">https://github.com/<wbr>infinispan/infinispan/pull/<wbr>3860</a>><br>
>> >>> > [2] <a href="https://github.com/infinispan/infinispan/pull/4348" rel="noreferrer" target="_blank">https://github.com/infinispan/<wbr>infinispan/pull/4348</a><br>
>> >>> <<a href="https://github.com/infinispan/infinispan/pull/4348" rel="noreferrer" target="_blank">https://github.com/<wbr>infinispan/infinispan/pull/<wbr>4348</a>><br>
>> >>><br>
>> >>><br>
>> >><br>
>> >><br>
>> ><br>
>> > --<br>
>> > Tristan Tarrant<br>
>> > Infinispan Lead<br>
>> > JBoss, a division of Red Hat<br>
>> > ______________________________<wbr>_________________<br>
>> > infinispan-dev mailing list<br>
>> > <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
>> > <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" rel="noreferrer" target="_blank">https://lists.jboss.org/<wbr>mailman/listinfo/infinispan-<wbr>dev</a><br>
>> ______________________________<wbr>_________________<br>
>> infinispan-dev mailing list<br>
>> <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" rel="noreferrer" target="_blank">https://lists.jboss.org/<wbr>mailman/listinfo/infinispan-<wbr>dev</a><br>
><br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> infinispan-dev mailing list<br>
> <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" rel="noreferrer" target="_blank">https://lists.jboss.org/<wbr>mailman/listinfo/infinispan-<wbr>dev</a><br>
______________________________<wbr>_________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" rel="noreferrer" target="_blank">https://lists.jboss.org/<wbr>mailman/listinfo/infinispan-<wbr>dev</a><br>
</div></div></blockquote></div><br></div>