Re: [infinispan-dev] How to improve our PR queue

[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(a)redhat.com >> <mailto:gfernand@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(a)redhat.com <mailto:slaskawi@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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev