[infinispan-dev] How to improve our PR queue

Tristan Tarrant ttarrant at redhat.com
Wed Oct 5 04:18:31 EDT 2016


[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


More information about the infinispan-dev mailing list