Hi All,
Please find the meeting notes as follows. Please feel free to correct
anything.
Problem - Patches are not looked in reasonable time. This is an
unsatisfactory situation for contributors especially if it involves
effort for creating patches. This is not a new problem. New experts and
bandwidth will not appear suddenly.
We want to collect thoughts about potential changes to the review
process. Can we have a possible solution where some patches can be kept
pending for months / years before it is eligible for review and
committing ?
Bertrand - Situation improved in last few days, The maintainers can edit
the commit message before committing instead of pushing back. It reduces
the probability of further series. Now there is too much activity
available on the mailing list.
Should we clean up the patches on the mailing list to distinguish
between the patches to be reviewed vs not to be reviewed ?
George - Sometimes, there is not very actionable feedback or too little
feedback. Lot of people are bypassing Andrew (unsure if the name is
correct) for review. There are not enough reviewers. Sometime people
unrelevant to the patch series are cc-ed. So the pipeline of a reviewer
increases. How to encourage new people to review ?
Diego - What is the bottleneck from Andrew’s perspective
George - Andrew has taken time to review past patch series. Sometime
security bugs are discovered quite late. Sometimes, reviewers are super
busy , so they don’t review. We can have meetings with maintainers where
people enjoy talking about review rather than writing. Then the reviewer
can explain the bug.
Roger has been stepping up as reviewer to offload Andrew.
Bertrand - Eg In PCI passthrough, Roger was not available so everything
was postponed. How to reduce bottlenecks? Can less experienced people
review , but risks introducing bugs. Accept that some feature might go
in with potential bugs (Feature marked unsupported), So end user can
understand risk. Later the unsupported features can be marked supported
when it is reviewed or tested.
Jan - Some person can review without being an explicit maintainer.
Bertrand - Some PCI passthrough issues are related to x86 . So we are
blocked by the x86 review. It is a case that Arm’s PCI passthrough
feature cannot be upstreamed.
George - Can there be a mentorship program , where new reviewers will be
paired with maintainers. And improve the review timelines.
Jan - Common code should be improved in all scenarios. What is in tree
already should not deteriote. Some of the Arm's PCI passthrough changes
introduces locking problem. Thus, we need to solve locking problem
before Arm’s changes are introduced.
Bertrand - In an unsupported, you have a first version. If too much
dependency is added in the first instance, then nothing progress.
Jurgen - Can we add experimental features with timeout. If no correction
done on an experimental feature, then the feature can be removed
Roger - Who is going to do cleanup
Bertrand - All the new code is bounded by configuration. Xen code is
modular. Easier to find what to remove.
George - Refactoring other parts of code will make it difficult
Jan - Some parts can still be scattered around. Can’t guard everything.
Bertrand - If 99% percent of code is modular, then probability of bugs
is reduced.
Anthony - Makefile changes took lot of time.
Bertrand - Differentiate between critical / non critical patches so we
can make faster progress.
Jan - When I rebase my tree, it took a day to solve problems introduced
Makefile changes. Do not blindly merge.
Anthony had to take time to make out of tree work.
George - If we wait until a series is perfected, then there is a lower
chance of bugs but lots of work/time. If we put things faster, then bugs
may be introduced and breaks. Need to find a sweet mid spot.
Jurgen - Working on the central boot process is hitting everyone.
Working on specific feature, only affects a limited set. Makefile
changes (by Anthony) affect everybody.
On PCI issues, the locking issues need to be fixed first. If the locking
is broken, then lot of side effects are introduced.
Bertrand - Spent lot of time discussion. We have an internal git branch
with lots of downstream patches. Now we are upstreaming the initial
features. Then x86 request came. No body looked at downstream gitlab repo.
Roger/Jan - No time to look at private gitlab repos.
Bertrand - How do we then fix things iteratively? Shouldn’t require
fixing everything in the first instance.
Jan - Regarding checkin new features. There might be limitations , but
bugs should not be checked in knowingly. FIXME should be used to fix
severe bugs. Locking problem is a severe bug to be fixed before. No one
had enough incentive to fix it. Unfortunate that Arm got affected.
EPAM is trying to fix the locking issues. Agree with Bertrand that
things moved slowly. Should not be bad bugs involved.
Someone - How do we guys deal with scope creep ? When the scope of the
initial work increases significantly
George - The work of refactoring of existing patch seems a lot of work
(sometime Arm guys don’t have x86 hardware/knowledge). Who is going to
do this ?
Jan - Ideally PCI should have been done with only Arm in mind.
George - All issue related to review bottleneck. If I need to review
something, ping me on irc. Sometime, things slip. If there was system
to assign a ticket to person for review (on gitlab). Other projects
have this philosophy
Jan - People shopuld be picking work, so they can do their best.
George - If the code is common with 6 mantainers, then sometimes I take
time and review. In the meantime someone else reviewes.
Jan - Double review is good. Different issues can be seen by different
person.
Bertrand - After one reviewers reviews the patch, should we send v2 or
wait for another reviewer to see v1. Can’t see the status of review on a
patch.
Jurgen - When I send a series for common code, Jan reviews , then I am
confident that no one else will review.
Sometimes, Jan takes few days.
Jan - No problems with a v2 but it address all the open questions on v1.
George - Have mentoring problem for reviewers. Maintainers can mentor
reviewer. Have a quiz to ask who will pick up the review.
Jurgen - Should I mention that I will do the review so that others will
know.
George - Know what I need to review from the mailing list. Then I will
look it.
Jurgen - Sometimes I review things which are related to linux. For other
parts which I am not confident, I leave it to review.
George - People learn by reviewing code. If they have a mentor, then
reviewers can do.
Jurgen - Reviewers may have a priority list for review.
Bertrand - Huge history in the mailing list. A rule that if my patch is
more than 2 months old, resend it
George - Have a system which will track who is reviewing with what
Jan - In my experience, pings do not work at all. If I don’t have
initial feedback why to send v2.
George - If a comment on v1 does not get addressed in 6 months, then
what happens.
You can check with maintainers to give feedback in due course of time.
Send a mail, review check it in two months and do it. Sometime series
get blocked on a single maintainer. Should not be that someone becomes a
bootleneck. We should offload things from people who are busy.
Jan - People should not be shy of looking at patches where maintainers
have already commented. Objections should expire if they are not
followed with concrete suggestion. So, other people can review and if
others agree (without maintainers) , then commit it.
Bertrand - New reviewers take time to have confidence for review.
Jan - If I get review from new people, there is some level of mistrust
if the patch series is intrusive. If the patch series is easy, then I
can trust review from new people. If new people review frequently, then
trust increases in due course of time. Also reviewers need to ask
questions or spot problems on the patch. No blind review.
Kind regards,
Ayan
