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