Re: Refactoring the Review Process

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux