On Code Reviews...
Patrick Williams
patrick at stwcx.xyz
Sun Sep 21 13:10:49 AEST 2025
Greetings,
This email is long. I'm not going to TL;DR it. If you contribute to
OpenBMC you should read it all.
# "Upstream is slow"
One thing I hear from many different people is "upstream code reviews
are slow". I see a few reasons for it seeming like they are so:
1. Code is not actually ready for review, based on how most reviews
and maintainers operate, but the author isn't aware and doesn't
know what to do next.
2. The bulk of the reviews are done by a few individuals and they
don't have the bandwidth to be quickly available for code
reviews.
3. We have some listed maintainers who are no longer actively
involved in the project.
I have some proposals to fix all of these problems (later).
# Data on reviewers
Since we collect data as part of the TOF election cycle, I can share
a finding.
Contributors Code Reviews Performed
------------ ----------------------
79 0
45 1-5
9 6-10
8 11-20
6 21-65
This is very disproportionate. 50% of the commits were reviewed by the
top 8 reviewers, 50% of the commits were reviewed by the remaining 139
contributors. 80% of the commits were reviewed by the top 23 reviewers
with 20% of the commits reviewed by the remaining 124 contributors.
If we want code changes to get merged, we absolutely have to have more
people helping with the code review burden!
Note:
This data reflects what we consider "high quality" reviews. That
means you didn't just give a token (+1) but you left some code
suggestions; a minimum of 3.
It is entirely unhelpful to go through and give a token (+1) on
commits without actually reviewing the change. This makes it so
that the maintainer still has to do all the review and will make
them stop trusting your reviews.
# Data on commits
I have another tool which can collect the current state of code reviews
(more on this later). This is a report of where we are currently at:
╭───────────┬───────────┬─────────────┬────────╮
│ ┆ Community ┆ Maintainers ┆ Author │
╞═══════════╪═══════════╪═════════════╪════════╡
│ <24 hours ┆ 3 ┆ 1 ┆ 2 │
├╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┤
│ <72 hours ┆ 64 ┆ 15 ┆ 66 │
├╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┤
│ <2 weeks ┆ 73 ┆ 24 ┆ 92 │
├╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┤
│ <8 weeks ┆ 43 ┆ 38 ┆ 132 │
├╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┤
│ >8 weeks ┆ 97 ┆ 34 ┆ 588 │
╰───────────┴───────────┴─────────────┴────────╯
Legend:
* "Author" - There is something wrong with this commit that needs to
be addressed by the author.
* "Community" - There has not been any review of this commit; the
expectation is that we have _someone_ other than the
maintainer do an initial review.
* "Maintainer" - The commit is "good to go" and has at least one
review. It is now waiting for final review and
[hopefully] merge.
Yes, we have almost 100 commits that _nobody_ has looked in at least 8
weeks. No wonder there is a perception of the process moving slow!
The vast majority of contributions are in "Author" state though. Even
commits under 2 weeks old, about 50% of them are seeking action from the
Author. There also appears to be tail of commits where once they reach 8
weeks old, in all likelihood, the author has given up.
I've heard from some people that they don't know what to review and they
don't know where to start. Clearly, we have plenty available for review.
We need to figure out how to make the connection.
# What's the solution?
AI... AI is the solution to all of our problems.
Just kidding... But, I have been working on a solution for most of this.
In Discord you'll see a new "openbmc-bot". This bot also serves a website
at https://openbmc.stwcx.xyz/bot . Feel free to pause and go there now.
## Change reminders
First up, to address the "I don't know what to review", openbmc-bot will
post every hour 5 commits that need to be reviewed into a new channel
`#code-review`. If one of those sound interesting, follow the link and
do a code review! You'll make the author happy (because even if you
give feedback, it's better than their change going to /dev/null,
right?), you'll help reduce the review burden on others, and _you_
might even learn something new about the project.
If none of those sound interesting, but you have some time to review,
follow the top link in the bot and you'll see every commit that needs
some review. Just pick one and go for it!
The trailing comment from the bot is "And there are XXX more...". Right
now we have over 200. Let's see how fast we can get that number at
least down to double digits?
Note:
Again, a reminder that doing a token (+1) isn't helpful. If you
really have read the change and the change really is good, feel free
to give a (+1). Don't just give a (+1) because you think you're
somehow helping or making someone's day. Make it meaningful.
## What's going on with my commit?
If you have a commit in Gerrit and you want to know what's going on with
it, go to the website, enter the Change ID and click "Change". This can
be a either a Gerrit short ID like 12345 or a full I-number. The page
will then tell you what the bot thinks is going on with your commit.
Maybe it is "Awaiting Community Review" or "Failing CI" or "Pending
Feedback to be Addressed"[1]. Hopefully this gives you a hint as to
what the next steps are.
Maybe you want to know what is going on with all your commits at once?
Go back to the main page, enter your Github username into the "Query"
box, and click "User". This will give you a report on every single
commit you have pending on Gerrit, who is responsible for the next
steps, and a ballpark of how long it has been waiting in that state.
If the change is in the "Author" category, that means _you_ are expected
to help push your own commit along. Maybe you have feedback to address,
maybe you have CI is failing, etc.
I know one status that might be confusing is "Pending Comment(s) to be
Addressed". If you have a comment that is "Unresolved" in Gerrit, your
change will get to this state. Usually that means someone gave you a
code review comment and they're expecting you to make a change.
Sometimes that means someone left a comment that you disagree with,
that's ok, but we are expecting the Author to drive consensus. If
you've responded, mark it resolved or reach out to the commenter or ask
one of the maintainers to help you out if you're unsure of what to do.
The net is when there is an open comment that means the ball is in your
(the author's court).
Lastly, there is a Discord slash command: `/obmc-review-status <id>`.
This is the same thing as the website but in Discord. Feel free to run
this in `#code-reviews` or open a private message to openbmc-bot and run
it there.
## What else can openbmc-bot do?
Off the main page there are a few more options:
* "All Open" - This will give a report of every pending commit in
Gerrit.
* "Per-Repo Summary" - This with show a table for every sub-project
in Gerrit with pending commits.
* "Project" - This is like the "User" query but for sub-projects.
If you are a maintainer of a sub-project, I think the "Project" query is
handy to get everything in the projects you maintain and see if there
are any pending commits for you to take action on.
## What about maintainers?
We do still have an issue of unresponsive maintainers. We've had some
informal discussions about this in the TOF and I think this half we
should take action. With the "Per-Repo Summary", we can see exactly
which repositories appear to be having maintainer issues.
We are absolutely in need of more maintainers. We've already documented
what the expectations and requirements are for becoming a maintainer[2].
This is a process and it takes time.
A big part of the requirements is that you need to demonstrate knowledge
of the repo _by doing code reviews_ and one of the biggest needs of the
project is people _doing code reviews_.
My suggestion if you're wanting to become a maintainer...
Pick a repositories that you're interested in and make it a habit of
regularly checking out the "Project" query for it. After you've done
a few of these and you feel comfortable, ask to be added as a reviewer
to the OWNERS file. After you continue to be comfortable, find a sponsor
for your addition as a maintainer (owner) in the OWNERS file. Of course,
it also helps if you make contributions, but over the course of doing
reviews you'll probably see little things that you see could use to be
cleaned up; start there if you don't have any "big feature" immediately
in mind.
If you see a repository in "Per-Repo Summary" that has a lot of
Community or Maintainer pending commits, it's probably a repository
that could use another maintainer or three.
# What's the deal with this bot?
I'm currently running this bot on a system of mine. I might move it to
the "Gerrit Server" at some point in the future. But, if it appears to
not be working, send me a message on `#infrastructure`.
I wrote this because I see a big need in the code review process, I
wanted something practical to write in Rust, and I've been doing a good
amount of "vibe-coding" lately anyhow.
The code is on Github[3] with a very unoriginal name and very little
documentation. Feel free to send PRs or open issues. If you want to
tell me how non-idiomatic my Rust is, that's fine too, but go ahead and
do that on Discord.
[1]: https://github.com/williamspatrick/gerrit-faster/blob/c6266398b3d8e220699d5b76867e82750fa163f3/src/changes/status.rs#L17
[2]: https://github.com/openbmc/docs/blob/master/community-membership.md#approver
[3]: https://github.com/williamspatrick/gerrit-faster
--
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20250920/f386c9be/attachment.sig>
More information about the openbmc
mailing list