<div dir="ltr">I agree with both Tristan and Sanne - they key point here is to make reviewing PRs a habit. Doing it once a day (or even more often as Tristan suggested) is a good place to start in my opinion.<div><br></div><div>The "rebase and merge" can speed the things up but it doesn't solve the main problem. We simply need to be more courageous when integrating PRs and avoid blocking project's progress.</div><div><br></div><div>Thanks</div><div>Sebastian</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 5, 2016 at 11:21 AM, Sanne Grinovero <span dir="ltr"><<a href="mailto:sanne@infinispan.org" target="_blank">sanne@infinispan.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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>
<span class="HOEnZb"><font color="#888888"><br>
Sanne<br>
</font></span><div class="HOEnZb"><div class="h5"><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>
</div></div></blockquote></div><br></div>