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

[Xen-devel] My notes on the process, review discussion at the developer meeting



 

Please find attached my notes from the process BoF and discussion at the developer meeting. Discussion and comments are welcome.

Lars  

 == Code Reviews ==

 The current workflow:

 We tend to have race conditions when it comes to reviews: the basic workflow is that reviewers tend to move patch reviews into a queue (typically a mail foloder) and then work through them. Some reviewers are faster/slower than others. For example: Jan and Ian are faster and tend to do most of the reviews. This then leads others – e.g. Stefano and George – to leave reviews to Jan and Ian.

 Discussion related to that topic:

- Stefano: if I started a review and then Ian/Jan respond to a review, I feel as if I wasted my time
- Jan: points out that reviews by multiple people are not a waste of time
- Stefano: this is more an issue of perception – often, I think I could have done something else from my list and take this into account the next time (aka do something else instead of a review). Do others have similar issues?
-
George: if some reviewers are faster, then I often ignore a review – aka the other series which has not yet been reviewed is the issue

- Lars: maybe we need to try to actively coordinate a bit more, given the increasing review load

- Stefano, Jan: If we did this, the trade off is that we are assigning work. Which Jan feels uncomfortable with.

 Revisiting the core issue:

It appears as if we are not using our review bandwidth as efficiently as we could

 Lars: I just checked and we do not have any documentation related to reviews.

- Wiki is not saying anything  (ACK BY – REVIEW BY)

  - Review BY does not need to come from maintainer

  - Jan: the process developed over time – also see http://markmail.org/message/ngbgi6z33hbxfx7z

- Jan: Maybe requiring people to do a number of reviews  to get their number of things committed?

- Stefano: Imperially it is feeling a lot slower than the 4 days longer review time that Lars identified from commit data

- Ian: queue folder also gets bigger

 Is there a tool that helps with keeping track who is reviewing what?

No, it appears there is not. E.g. Gerrit doesn't do it either. Patchwork can’t tell whether a new version of a series has been posted.

 A related problem is that a thorough review (or someone new entering the review process) later in the series hurts us

- Reviewers who come in after 3-4 stages, often cause disruption if changes are cosmetic and no real bugs are found

- Cosmetic changes should me made in early versions of patch series

- Jan: as a maintainer/committer one should push back on cosmetic changes when found later in a series

- Stefano: Partial reviews are hurting us more than other reviews. In these cases, we review the first bit of a series. Then we get interrupted by another activity – e.g. for a week – you don’t look at the code. Then submitter re-sends and the entire series may have changed, causing extra work for both the submitter and the reviewer.

 Proposal for Best Practice: Write a guideline document to try to be more efficient and help newcomers contribute or review patch series:

- Encourage contributors to never be shy of doing reviews

- Do architectural reviews as early as possible. At v6 only review for bug fixes

- Try to review the full series as early as possible, reduce partial reviews.

- Add reviewed-by of people that did internal reviews. The background here is that many organizations do internal reviews before the code hits xen-devel. Konrad: REVIEWED BY INTERNALLY is less valuable – Linuses viewpoint is that internal reviews should be ignored. We then had a brief discussion about it, and how much we value an internal review depends on how well the internal reviewers are known (the same as for external reviews)

- Tell on IRC when you intend to review a big patch series.

- Try to CC the author of the code, not necessarily the maintainer.

- Add a tag on the subject to explain what the patch is about (x86, arm, libxl).

- Maintainers should encourage people to appropriately tag their emails.

- We encourage readers to add filters to the list to be able to follow what they want.

- If someone is willing to review, encourage them to state that I am willing to review x amount of things a week - a team leader/maintainer could then suggest a patch to review. Fear that this mechanism creates extra mailing list traffic.

- The document should define clearly what ACKY BY REVIEW means and when to do reviews

- A related idea is to propose some kind of mentoring structure, for people who like to mentor and those who are willing to look for mentorship

 

== Mailing List Traffic ==

 Problems: Mailing List traffic = making hard for newcomers to get started

- Xen + QEMU – lots of email and distraction

- New people don’t know what’s important and what isn’t and where to start

- Subject doesn’t always match the area – so filtering does not always work

 Lars: could we use some auto-tagging based on code paths in the email body (somewhere on the mail backend)

 The concern that was raised was that it would make subject lines too long. Maybe we can start to encourage people to use better tags.

 We had a brief discussion on whether we should follow the Linux model, where there is a hierarchy of lists.

 Stefano: Xen on ARM – filtering does actually work, because maintainers enforce CC.  Linux uses different lists for ARM, kernel, drivers, …

 If we did this, we would end up with 3 or 4 lists if we did it (could come up with some buckets: tools, core, x86, arm) … the danger is that we would end up with false positives.

Also, there seem to be some wrong tagging with some mail clients when mail is sent to several lists

 ACTION: Lars to experiment and see what actually happens

 Lars: maybe it is too early to try and resolve this. We could look into how this works with the embedded and automotive list and see how well this works further down the line

== Developer Events ==

From a sponsorship viewpoint having two separate events is less than ideal. It is cheaper and easier to find sponsors if we have a combined event. My proposal would be to

- Join Dev and User event into a 3 day event. The goal would be to run the developer part as we always have, but add an extra day for users. The 3rd day can then be used for developer meetings, board meetings and working group meetings

- We would leave the Hackathons as they are, but George suggested that maybe we need a better name as in fact the Hackathons are npot Hackathons (suggestion: Developer Meeting)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.