Refactoring the Review Process

Refactoring the Review Process

[Darrick J. Wong]

Hi everyone,

Writing and reviewing code in isolation hasn't always served me well.  I
really enjoyed my experiences developing the reflink code (~2015) being
able to chat with Dave in the evenings about the design of particular
algorithms, or how certain XFS structures really worked, and to learn
the history behind this and that subsystem.

Returning to first principles, I perceive that the purpose of our review
processes is to make sure there aren't any obvious design flaws or
implementation errors in the code we put back to the git repo by
ensuring that at least one other XFS developer actually understands
what's going on.

In other words, I am interested in testing the pair programming
paradigm.  Given that we have zero physical locality, I suspect this
will work better with an interactive medium and between people who are
in nearby time zones.  I also suspect that this might be better used for
more focussed activities such as code walkthroughs and reviews.  Still,
I'm willing to entertain the possibility of using this as a second means
to get a patchset to a Reviewed-by.

I also speculate that this might be a good mentoring opportunity for us
to trade productivity tips and disseminate 'institutional' knowledge
between people.  I for one am happy to help others learn more about the
code base in exchange for learning more about the parts of XFS with
which I'm less familiar.  (I bet Allison knows more about how xattrs
work than I do at this point...)

--D

Re: Refactoring the Review Process

[Eric Sandeen]

On 4/22/20 5:25 PM, Darrick J. Wong wrote:
> Hi everyone,
> 
> Writing and reviewing code in isolation hasn't always served me well.  I
> really enjoyed my experiences developing the reflink code (~2015) being
> able to chat with Dave in the evenings about the design of particular
> algorithms, or how certain XFS structures really worked, and to learn
> the history behind this and that subsystem.
> 
> Returning to first principles, I perceive that the purpose of our review
> processes is to make sure there aren't any obvious design flaws or
> implementation errors in the code we put back to the git repo by
> ensuring that at least one other XFS developer actually understands
> what's going on.
> 
> In other words, I am interested in testing the pair programming
> paradigm.  Given that we have zero physical locality, I suspect this
> will work better with an interactive medium and between people who are
> in nearby time zones.  I also suspect that this might be better used for
> more focussed activities such as code walkthroughs and reviews.  Still,
> I'm willing to entertain the possibility of using this as a second means
> to get a patchset to a Reviewed-by.
> 
> I also speculate that this might be a good mentoring opportunity for us
> to trade productivity tips and disseminate 'institutional' knowledge
> between people.  I for one am happy to help others learn more about the
> code base in exchange for learning more about the parts of XFS with
> which I'm less familiar.  (I bet Allison knows more about how xattrs
> work than I do at this point...)

Just to chime in, I like this idea.  I think that it's a fairly open-ended
suggestion, as this could be anything from:

* True, traditional pair-programming for new work
* Dividing up tasks for larger projects, and cross-reviewing each others work
* High-bandwidth review of existing work by interacting directly w/ patch author
  in real time on irc or video
* ??? other ideas

In the first couple cases, this might result on patches first appearing on the
list with a "pre-existing" Reviewed-by that came out of that teamwork.

As a result we should probably consider a policy that patches remain on the list
for at least $X days to allow further comment by "3rd parties" if the list is to
remain (as it should) the ultimate forum for patch acceptance.

The other thought I have is that while pairing in any of these ways will be an
excellent growth opportunity (in general, or in a specific area of code) we should
be somewhat mindful of "lopsided" pairing, i.e. having a guru paired with a total
newbie for an extremely large, complex project may not be the best idea, but that's
probably self-evident.  Such teamwork will probably somewhat naturally select into
logical pairings depending on the task at hand.

So, for myself, if anyone wants to review anything I've written in real time, or
would like me to collaborate on their upcoming work, I'm happy to do so within
the limits of my schedule and bandwidth.  :)

Thanks,
-Eric

Re: Refactoring the Review Process

[Allison Collins]

On 4/23/20 7:11 AM, Eric Sandeen wrote:
> On 4/22/20 5:25 PM, Darrick J. Wong wrote:
>> Hi everyone,
>>
>> Writing and reviewing code in isolation hasn't always served me well.  I
>> really enjoyed my experiences developing the reflink code (~2015) being
>> able to chat with Dave in the evenings about the design of particular
>> algorithms, or how certain XFS structures really worked, and to learn
>> the history behind this and that subsystem.
>>
>> Returning to first principles, I perceive that the purpose of our review
>> processes is to make sure there aren't any obvious design flaws or
>> implementation errors in the code we put back to the git repo by
>> ensuring that at least one other XFS developer actually understands
>> what's going on.
>>
>> In other words, I am interested in testing the pair programming
>> paradigm.  Given that we have zero physical locality, I suspect this
>> will work better with an interactive medium and between people who are
>> in nearby time zones.  I also suspect that this might be better used for
>> more focussed activities such as code walkthroughs and reviews.  Still,
>> I'm willing to entertain the possibility of using this as a second means
>> to get a patchset to a Reviewed-by.
>>
>> I also speculate that this might be a good mentoring opportunity for us
>> to trade productivity tips and disseminate 'institutional' knowledge
>> between people.  I for one am happy to help others learn more about the
>> code base in exchange for learning more about the parts of XFS with
>> which I'm less familiar.  (I bet Allison knows more about how xattrs
>> work than I do at this point...)
I feel I have a decent understanding at this point, but the challenge 
atm is making it look pretty so it's not so hard for other to look at 
too.  In any case, I'd be happy to walk you through it some time :-)

> 
> Just to chime in, I like this idea.  I think that it's a fairly open-ended
> suggestion, as this could be anything from:
> 
> * True, traditional pair-programming for new work
> * Dividing up tasks for larger projects, and cross-reviewing each others work
> * High-bandwidth review of existing work by interacting directly w/ patch author
>    in real time on irc or video
> * ??? other ideas
I used to work on a team that was very fond of "screen" sessions, though 
maybe limited to people of a common org for vpn reasons.

> 
> In the first couple cases, this might result on patches first appearing on the
> list with a "pre-existing" Reviewed-by that came out of that teamwork.
> 
> As a result we should probably consider a policy that patches remain on the list
> for at least $X days to allow further comment by "3rd parties" if the list is to
> remain (as it should) the ultimate forum for patch acceptance.
That makes sense :-)

> 
> The other thought I have is that while pairing in any of these ways will be an
> excellent growth opportunity (in general, or in a specific area of code) we should
> be somewhat mindful of "lopsided" pairing, i.e. having a guru paired with a total
> newbie for an extremely large, complex project may not be the best idea, but that's
> probably self-evident.  Such teamwork will probably somewhat naturally select into
> logical pairings depending on the task at hand.
> 
> So, for myself, if anyone wants to review anything I've written in real time, or
> would like me to collaborate on their upcoming work, I'm happy to do so within
> the limits of my schedule and bandwidth.  :)
> 
> Thanks,
> -Eric
> 

Yes, I think it would be a beneficial idea too.  Some of the prior teams 
I worked on would have internal reviews before sending things to the 
mailing lists.  It seemed to have the effect of things being a little 
more picked over before sending things to a wider scope of review.  Sort 
of like a low level phase of nits, before going to higher level phase of 
architecture.  It also had the effect of the internal reviewers better 
understanding the design intent before it goes out.  So they tended to 
be more engaged the high level discussion and less engaged on the nits. 
Comparatively it doesn't feel like we have as much division here, so 
everything just happens all at once and I think sometimes people get a 
bit lost in it all.  So perhaps the pair programing approach may help to 
bring a little more structure to our dev process.

Overtime, I suspect the 1x1's may also have the effect of peoples dev 
habits rubbing off on each other too.  Which in general tends to 
generate less things to have a difference of opinion about (in terms of 
design or styles).  People also tend to adopt similar configs or dev 
setups which helps to better pre-disposition them to help each other 
with it later.

Allison