Quantcast

[DISCUSS] Shepherding PRs

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

[DISCUSS] Shepherding PRs

andy
Administrator
I thought this email exchange from the Mesos dev list was worth sharing.
The Mesos project is trying out a process wherein they assign shepherds
(who are committers) to significant issues.

I'm not proposing that this necessarily makes sense for us, but I thought
it might be worth discussing.

Andy
---------- Forwarded message ----------
From: "Benjamin Mahler" <[hidden email]>
Date: Mar 24, 2014 11:47 PM
Subject: Re: Shepherding on ExternalContainerizer
To: "dev" <[hidden email]>
Cc:

Hey Till,

We want to foster a healthy review culture, and so, as you observed, we
thought we would try out the notion of having a "shepherd" for each review.

In the past we've had some reviews stagnate because there was no clear
accountability for getting it committed. Meaning, various committers would
be included in the 'Reviewers' and each would provide feedback
independently, but there was no single person accountable for "shepherding"
the change to a shippable state, and ultimately committing it.

We've also had issues with having a lot of lower value reviews crowding out
higher value reviews. Often these lower value reviews are things like
cleanup, refactoring, etc, which tend to be easier to review. Shepherding
doesn't address this as directly, but it is also an effort to ensure we
balance low value changes (technical debt, refactoring, cleanup, etc) with
higher value changes (features, bug fixes, etc) via shepherd assignment.

This is why we've been trying out the "shepherd" concept.

Related to this (and *not* related to your changes Till :)), I would
encourage two behaviors from "reviewees" to ameliorate the situation:

1. Please be cognizant of the fact that reviewing tends to be a bottleneck
and that reviewer time is currently at a premium. This means, please be
very thorough in your work and also look over your patches before sending
them out. This saves your time (faster reviews) and reviewers' time (fewer
comments needed). Feel free to reach out for feedback before sending out
reviews as well (if feasible).

2. Also, be cognizant of the fact that we need to balance low and high
priority reviews. Sometimes we don't have time to review low value cleanup
work when there are a lot of things in flight. For example, I have a bunch
of old cleanup patches from when we need to get more important things
committed, and I know Vinod has old cleanup patches like this as well.

This all being said, the external containerizer is high value and should
definitely be getting reviews. I will take some time to go over your
changes later this week with Ian, when I'll be free from a deadline ;). We
can help "pair shepherd" your changes.

Ben


On Mon, Mar 24, 2014 at 4:32 PM, Till Toenshoff <[hidden email]> wrote:

> Dear Devs/Committers,
>
> after having developed the ExternalContainerizer, I am now obviously eager
> to get it committed. After receiving and addressing a couple of comments
> (thanks @all who commented - that helped a lot), I now am once again in a
> stage of waiting and keeping fingers crossed that my patch won't need
> rebasing before someone has a thorough look at it. I do appreciate and
> fully understand the fact that you committers are under heavy load.
>
> By experience and seeing some RR comments, I learned that there appears to
> be a new entity in our review process; a "shepherd". Sounds like a great
> idea, even though I am not entirely sure what that means in detail for
> Mesos. I guess that is something that makes sure that final commit
> decisions  are done by a single voice, preventing contradicting comments
> etc... Knowing that other projects actually demand the patch-submitter to
ask
> for shepherding, I figured why not doing the same.
>
> For that ExternalContainerizer baby, I would kindly like to call out for a
> shepherd. Guessing that a shepherd needs to be a committer but also
knowing
> that Ian is very deeply involved within containerizing, I would like to
> "nominate" Niklas as a committer in collaboration with Ian. Hope that
makes
> sense and don't hesitate to tell me that this was not the right way to
> achieve shepherding.
>
> cheers!
> Till
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Shepherding PRs

Evan Chan
+1.

I really like this idea.  I know having a shepherd would have been really
helpful for a couple of changes.


On Thu, Mar 27, 2014 at 8:18 AM, Andy Konwinski <[hidden email]>wrote:

> I thought this email exchange from the Mesos dev list was worth sharing.
> The Mesos project is trying out a process wherein they assign shepherds
> (who are committers) to significant issues.
>
> I'm not proposing that this necessarily makes sense for us, but I thought
> it might be worth discussing.
>
> Andy
> ---------- Forwarded message ----------
> From: "Benjamin Mahler" <[hidden email]>
> Date: Mar 24, 2014 11:47 PM
> Subject: Re: Shepherding on ExternalContainerizer
> To: "dev" <[hidden email]>
> Cc:
>
> Hey Till,
>
> We want to foster a healthy review culture, and so, as you observed, we
> thought we would try out the notion of having a "shepherd" for each review.
>
> In the past we've had some reviews stagnate because there was no clear
> accountability for getting it committed. Meaning, various committers would
> be included in the 'Reviewers' and each would provide feedback
> independently, but there was no single person accountable for "shepherding"
> the change to a shippable state, and ultimately committing it.
>
> We've also had issues with having a lot of lower value reviews crowding out
> higher value reviews. Often these lower value reviews are things like
> cleanup, refactoring, etc, which tend to be easier to review. Shepherding
> doesn't address this as directly, but it is also an effort to ensure we
> balance low value changes (technical debt, refactoring, cleanup, etc) with
> higher value changes (features, bug fixes, etc) via shepherd assignment.
>
> This is why we've been trying out the "shepherd" concept.
>
> Related to this (and *not* related to your changes Till :)), I would
> encourage two behaviors from "reviewees" to ameliorate the situation:
>
> 1. Please be cognizant of the fact that reviewing tends to be a bottleneck
> and that reviewer time is currently at a premium. This means, please be
> very thorough in your work and also look over your patches before sending
> them out. This saves your time (faster reviews) and reviewers' time (fewer
> comments needed). Feel free to reach out for feedback before sending out
> reviews as well (if feasible).
>
> 2. Also, be cognizant of the fact that we need to balance low and high
> priority reviews. Sometimes we don't have time to review low value cleanup
> work when there are a lot of things in flight. For example, I have a bunch
> of old cleanup patches from when we need to get more important things
> committed, and I know Vinod has old cleanup patches like this as well.
>
> This all being said, the external containerizer is high value and should
> definitely be getting reviews. I will take some time to go over your
> changes later this week with Ian, when I'll be free from a deadline ;). We
> can help "pair shepherd" your changes.
>
> Ben
>
>
> On Mon, Mar 24, 2014 at 4:32 PM, Till Toenshoff <[hidden email]> wrote:
>
> > Dear Devs/Committers,
> >
> > after having developed the ExternalContainerizer, I am now obviously
> eager
> > to get it committed. After receiving and addressing a couple of comments
> > (thanks @all who commented - that helped a lot), I now am once again in a
> > stage of waiting and keeping fingers crossed that my patch won't need
> > rebasing before someone has a thorough look at it. I do appreciate and
> > fully understand the fact that you committers are under heavy load.
> >
> > By experience and seeing some RR comments, I learned that there appears
> to
> > be a new entity in our review process; a "shepherd". Sounds like a great
> > idea, even though I am not entirely sure what that means in detail for
> > Mesos. I guess that is something that makes sure that final commit
> > decisions  are done by a single voice, preventing contradicting comments
> > etc... Knowing that other projects actually demand the patch-submitter to
> ask
> > for shepherding, I figured why not doing the same.
> >
> > For that ExternalContainerizer baby, I would kindly like to call out for
> a
> > shepherd. Guessing that a shepherd needs to be a committer but also
> knowing
> > that Ian is very deeply involved within containerizing, I would like to
> > "nominate" Niklas as a committer in collaboration with Ian. Hope that
> makes
> > sense and don't hesitate to tell me that this was not the right way to
> > achieve shepherding.
> >
> > cheers!
> > Till
> >
> >
>



--
--
Evan Chan
Staff Engineer
[hidden email]  |

<http://www.ooyala.com/>
<http://www.facebook.com/ooyala><http://www.linkedin.com/company/ooyala><http://www.twitter.com/ooyala>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Shepherding PRs

Mark Hamstra
So what happens when having a shepherd is not helpful, e.g. when the
designated shepherd gets encumbered with a pile of higher-priority
responsibilities while the PR being shepherded is still in process?


On Thu, Mar 27, 2014 at 10:12 AM, Evan Chan <[hidden email]> wrote:

> +1.
>
> I really like this idea.  I know having a shepherd would have been really
> helpful for a couple of changes.
>
>
> On Thu, Mar 27, 2014 at 8:18 AM, Andy Konwinski <[hidden email]
> >wrote:
>
> > I thought this email exchange from the Mesos dev list was worth sharing.
> > The Mesos project is trying out a process wherein they assign shepherds
> > (who are committers) to significant issues.
> >
> > I'm not proposing that this necessarily makes sense for us, but I thought
> > it might be worth discussing.
> >
> > Andy
> > ---------- Forwarded message ----------
> > From: "Benjamin Mahler" <[hidden email]>
> > Date: Mar 24, 2014 11:47 PM
> > Subject: Re: Shepherding on ExternalContainerizer
> > To: "dev" <[hidden email]>
> > Cc:
> >
> > Hey Till,
> >
> > We want to foster a healthy review culture, and so, as you observed, we
> > thought we would try out the notion of having a "shepherd" for each
> review.
> >
> > In the past we've had some reviews stagnate because there was no clear
> > accountability for getting it committed. Meaning, various committers
> would
> > be included in the 'Reviewers' and each would provide feedback
> > independently, but there was no single person accountable for
> "shepherding"
> > the change to a shippable state, and ultimately committing it.
> >
> > We've also had issues with having a lot of lower value reviews crowding
> out
> > higher value reviews. Often these lower value reviews are things like
> > cleanup, refactoring, etc, which tend to be easier to review. Shepherding
> > doesn't address this as directly, but it is also an effort to ensure we
> > balance low value changes (technical debt, refactoring, cleanup, etc)
> with
> > higher value changes (features, bug fixes, etc) via shepherd assignment.
> >
> > This is why we've been trying out the "shepherd" concept.
> >
> > Related to this (and *not* related to your changes Till :)), I would
> > encourage two behaviors from "reviewees" to ameliorate the situation:
> >
> > 1. Please be cognizant of the fact that reviewing tends to be a
> bottleneck
> > and that reviewer time is currently at a premium. This means, please be
> > very thorough in your work and also look over your patches before sending
> > them out. This saves your time (faster reviews) and reviewers' time
> (fewer
> > comments needed). Feel free to reach out for feedback before sending out
> > reviews as well (if feasible).
> >
> > 2. Also, be cognizant of the fact that we need to balance low and high
> > priority reviews. Sometimes we don't have time to review low value
> cleanup
> > work when there are a lot of things in flight. For example, I have a
> bunch
> > of old cleanup patches from when we need to get more important things
> > committed, and I know Vinod has old cleanup patches like this as well.
> >
> > This all being said, the external containerizer is high value and should
> > definitely be getting reviews. I will take some time to go over your
> > changes later this week with Ian, when I'll be free from a deadline ;).
> We
> > can help "pair shepherd" your changes.
> >
> > Ben
> >
> >
> > On Mon, Mar 24, 2014 at 4:32 PM, Till Toenshoff <[hidden email]>
> wrote:
> >
> > > Dear Devs/Committers,
> > >
> > > after having developed the ExternalContainerizer, I am now obviously
> > eager
> > > to get it committed. After receiving and addressing a couple of
> comments
> > > (thanks @all who commented - that helped a lot), I now am once again
> in a
> > > stage of waiting and keeping fingers crossed that my patch won't need
> > > rebasing before someone has a thorough look at it. I do appreciate and
> > > fully understand the fact that you committers are under heavy load.
> > >
> > > By experience and seeing some RR comments, I learned that there appears
> > to
> > > be a new entity in our review process; a "shepherd". Sounds like a
> great
> > > idea, even though I am not entirely sure what that means in detail for
> > > Mesos. I guess that is something that makes sure that final commit
> > > decisions  are done by a single voice, preventing contradicting
> comments
> > > etc... Knowing that other projects actually demand the patch-submitter
> to
> > ask
> > > for shepherding, I figured why not doing the same.
> > >
> > > For that ExternalContainerizer baby, I would kindly like to call out
> for
> > a
> > > shepherd. Guessing that a shepherd needs to be a committer but also
> > knowing
> > > that Ian is very deeply involved within containerizing, I would like to
> > > "nominate" Niklas as a committer in collaboration with Ian. Hope that
> > makes
> > > sense and don't hesitate to tell me that this was not the right way to
> > > achieve shepherding.
> > >
> > > cheers!
> > > Till
> > >
> > >
> >
>
>
>
> --
> --
> Evan Chan
> Staff Engineer
> [hidden email]  |
>
> <http://www.ooyala.com/>
> <http://www.facebook.com/ooyala><http://www.linkedin.com/company/ooyala><
> http://www.twitter.com/ooyala>
>
Loading...