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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Tristan Tarrant-2
[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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sanne Grinovero-3
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 <[hidden email]> 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 <[hidden email]
>>> <mailto:[hidden email]>> 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
>>>     <[hidden email] <mailto:[hidden email]>> 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
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sebastian Laskawiec
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]
>>> <mailto:[hidden email]>> 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
>>>     <[hidden email] <mailto:[hidden email]>> 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
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dan Berindei
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
<[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]
>> >>> <mailto:[hidden email]>> 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
>> >>>     <[hidden email] <mailto:[hidden email]>> 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
>> > [hidden email]
>> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vojtech Juranek
In reply to this post by Tristan Tarrant-2
On Wednesday 05 October 2016 10:18:31 Tristan Tarrant wrote:
> - PRs opened by external developers won't have the appropriate labels
> for permission reasons. Add them.


would be nice if the labels can be reset automatically once PR is updated.  
Labels are usually outdated and misleading after PR update and reviewer has to
check the comments under PR (in better case, in worse the PR is ignored as it
has "Changes required" label or similar)
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev

signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sebastian Laskawiec
In reply to this post by Dan Berindei
+1 - for the last time :D

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...

On Wed, Oct 5, 2016 at 11:51 AM, Dan Berindei <[hidden email]> wrote:
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
<[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]
>> >>> <mailto:[hidden email]>> 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
>> >>>     <[hidden email] <mailto:[hidden email]>> 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
>> > [hidden email]
>> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Radim Vansa
In reply to this post by Vojtech Juranek
On 10/05/2016 12:26 PM, Vojtech Juranek wrote:
> On Wednesday 05 October 2016 10:18:31 Tristan Tarrant wrote:
>> - PRs opened by external developers won't have the appropriate labels
>> for permission reasons. Add them.
>
> would be nice if the labels can be reset automatically once PR is updated.
> Labels are usually outdated and misleading after PR update and reviewer has to
> check the comments under PR (in better case, in worse the PR is ignored as it
> has "Changes required" label or similar)

This is only a problem for PR authors who don't have write-permissions
on the PR to update it. A workaround is to ask on IRC to update the
labels; proper solution is a webhook (probably to a script on
infinispan.org) that will change the labels accordingly.

Radim

>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Tristan Tarrant-2
In reply to this post by Dan Berindei
On 05/10/16 11:51, Dan Berindei wrote:
> 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.
The only place I like using the thumbs up is when I'm the author and I'm
agreeing with the reviewers comment.
It has the useful side effect of keeping track of all the reviews thus
limiting the chance I might miss something

Tristan
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Loading...