Task #2089
Encourage code review
Description
Create incentives to encourage code review and improve turn-around time for it
One proposed idea is an opt-in quid-per-quo model. Anyone opting in would promise to review one change created by the reviewer in return. One should aim for taking as much time as the reviewer did (picking an as complex commit (or multiple small ones)) and should make it high priority so that one can do this review reasonable quickly. Both the complexity and time-frame are intentional kept vague because it is meant as an honor system.
This redmine is indented to collect possible ideas for incentives and to opt-in to any incentives anyone wants to support.
History
#1 Updated by Vedran Miletic over 3 years ago
Roland Schulz wrote:
One proposed idea is an opt-in quid-per-quo model. Anyone opting in would promise to review one change created by the reviewer in return. One should aim for taking as much time as the reviewer did (picking an as complex commit (or multiple small ones)) and should make it high priority so that one can do this review reasonable quickly. Both the complexity and time-frame are intentional kept vague because it is meant as an honor system.
Sounds like a good idea. How would we measure commit complexity, perhaps number of removed + number of added lines?
#2 Updated by David van der Spoel over 3 years ago
How about "code-reviewer of the month" awards?
#3 Updated by Szilárd Páll over 3 years ago
David van der Spoel wrote:
How about "code-reviewer of the month" awards?
I like the idea.
As noted during the telco, I think the following would also help, but I'm not sure what do we have interest/resources for:
- reduce gerrit spam: auto-invite for review is counter-productive, more configurable emails (e.g. per change digest?)
- improve sorting: e.g. counteract to some degree the bias of very update heavy (e.g. WIP but not tagged so), changes that will end up getting priority due to the date-based sorting defaults;
- tagging (gmail-style: multiple topics, requirements, know-how)
- "top 5 changes of the week" (to review) posts on gmx-dev?
#4 Updated by Roland Schulz over 3 years ago
Top Reviewers of the year by comments(1): Mark Abraham | 1128 Berk Hess | 612 Teemu Murtola | 509 Roland Schulz | 494 Erik Lindahl | 413 Szilárd Páll | 344 David van der Spoel | 303 Aleksei Iupinov | 112 Magnus Lundborg | 101 Carsten Kutzner | 70 Top Reviewers of the month by comments(2): Roland Schulz | 53 Mark Abraham | 35 Berk Hess | 34 Aleksei Iupinov | 28 Teemu Murtola | 27 Magnus Lundborg | 20 Erik Lindahl | 10 Vedran Miletic | 8 Eric Irrgang | 8 Szilárd Páll | 7 Top Reviewers of the year by voted commits(3): Mark Abraham | 329 Berk Hess | 138 Erik Lindahl | 114 Teemu Murtola | 104 David van der Spoel | 102 Szilárd Páll | 79 Roland Schulz | 70 Aleksei Iupinov | 60 Magnus Lundborg | 31 Carsten Kutzner | 17 Top Reviewers of the month by voted commits(4): Aleksei Iupinov | 19 Roland Schulz | 16 Mark Abraham | 14 Berk Hess | 11 Magnus Lundborg | 10 Teemu Murtola | 8 Vedran Miletic | 4 Szilárd Páll | 3 David van der Spoel | 3 Eric Irrgang | 2
Number of comments count any time one creates a new comment or vote on a commit. If one creates comments on multiple lines this still only counts as one.
Number of voted commits count the total number of code review votes on unique commits.
Obviously this can't measure the quality of the review or the complexity of the code reviewed. One could take the SLOC in the change into account, but that's not trivial to do and I'm not sure how useful it would be.
If we want to have a reviewer of the month by quality we probably should vote on it.
Please let me know if you are interested in further statistics.
1) select Full_Name, count(*) as reviewed from PATCH_SET_APPROVALS as P, Accounts as A, Changes as C
where CATEGORY_ID!='SUBM' and A.ACCOUNT_ID=P.ACCOUNT_ID and C.change_id=P.change_id and OWNER_ACCOUNT_ID != A.ACCOUNT_ID and DEST_PROJECT_NAME in ('gromacs', 'manual', 'regressiontests') and GRANTED>'2016-1-1 0:0:0' and GRANTED<'2017-1-1 0:0:0' and Full_Name!='Jenkins Buildbot' group by Full_Name order by reviewed desc limit 10;
2) select Full_Name, count(*) as reviewed from PATCH_SET_APPROVALS as P, Accounts as A, Changes as C
where CATEGORY_ID!='SUBM' and A.ACCOUNT_ID=P.ACCOUNT_ID and C.change_id=P.change_id and OWNER_ACCOUNT_ID != A.ACCOUNT_ID and DEST_PROJECT_NAME in ('gromacs', 'manual', 'regressiontests') and GRANTED>'2016-12-1 0:0:0' and GRANTED<'2017-1-1 0:0:0' and Full_Name!='Jenkins Buildbot' group by Full_Name order by reviewed desc limit 10;
3) select Full_Name, count(distinct P.change_id) as reviewed from PATCH_SET_APPROVALS as P, Accounts as A, Changes as C
where CATEGORY_ID='Code-Review' and A.ACCOUNT_ID=P.ACCOUNT_ID and C.change_id=P.change_id and OWNER_ACCOUNT_ID != A.ACCOUNT_ID and DEST_PROJECT_NAME in ('gromacs', 'manual', 'regressiontests') and GRANTED>'2016-1-1 0:0:0' and GRANTED<'2017-1-1 0:0:0' and value!=0 and Full_Name!='Jenkins Buildbot' group by Full_Name order by reviewed desc limit 10;
4) select Full_Name, count(distinct P.change_id) as reviewed from PATCH_SET_APPROVALS as P, Accounts as A, Changes as C
where CATEGORY_ID='Code-Review' and A.ACCOUNT_ID=P.ACCOUNT_ID and C.change_id=P.change_id and OWNER_ACCOUNT_ID != A.ACCOUNT_ID and DEST_PROJECT_NAME in ('gromacs', 'manual', 'regressiontests') and GRANTED>'2016-12-1 0:0:0' and GRANTED<'2017-1-1 0:0:0' and value!=0 and Full_Name!='Jenkins Buildbot' group by Full_Name order by reviewed desc limit 10;
#5 Updated by Roland Schulz over 3 years ago
If someone is interested, I could also compile a table with the ratio of commits voted on to commits uploaded. Since we require at least 2 votes per commit we need that ratio to be over 2 for gerrit to not clog up.
#6 Updated by David van der Spoel over 3 years ago
I posted the first table on GROMACS Facebook page yesterday with an encouragement to become code-reviewers. Lots of likes. Not sure whether new people did register...
#7 Updated by Teemu Murtola over 3 years ago
The last two queries clearly do not return what intended. The last one has a typo where it returns all non-code-review actions, and both seem to return also count changes where the user has not voted (since the PATCH_SET_APPROVALS table is full of rows with Code-Review 0 entries, and the first queries even seem to rely on counting these rows as equivalent to comments).
#8 Updated by Teemu Murtola over 3 years ago
PS. The last time the code review statistics were on the table (a bit over two years back), I wrote a Python script that computes all kinds of statistics, and can be used, verified, and adapted by anyone (as opposed to the SQL queries, which require Gerrit admin permissions). But unfortunately they no longer work, because the gerrit query SSH command nowadays apparently hangs indefinitely with larger result sets...
#9 Updated by Roland Schulz over 3 years ago
Thanks! Of course you are right. I update the queries and the table.
#10 Updated by Roland Schulz over 3 years ago
In the gerrit log I find:
[2017-01-06 07:07:19,997] [SSH gerrit query --format=JSON --all-approvals --comments -- -age:30d OR status:open (tmurtola)] ERROR com.google.gerrit.sshd.BaseCommand : Internal server error (user tmurtola account 1000037) during gerrit query --format=JSON --all-approvals --comments -- -age:30d OR status:open
org.apache.sshd.common.SshException: flush(ChannelOutputStream[ChannelSession[id=0, recipient=0]-ServerSessionImpl[tmurtola@/78.27.120.189:41002]] SSH_MSG_CHANNEL_DATA) length=8192 - stream is already closed
at org.apache.sshd.common.channel.ChannelOutputStream.flush(ChannelOutputStream.java:163)
at com.google.gerrit.sshd.BaseCommand$TaskThunk.run(BaseCommand.java:449)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
at java.util.concurrent.FutureTask.run(FutureTask.java:262)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:178)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:292)
at com.google.gerrit.server.git.WorkQueue$Task.run(WorkQueue.java:417)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
[2017-01-06 07:27:38,144] [sshd-SshServer[212c74c0]-nio2-thread-2] WARN org.apache.sshd.server.session.ServerSessionImpl : exceptionCaught(ServerSessionImpl[null@/163.172.233.70:36482])[state=Opened] IOException: Connection reset by peer
If I execute:
ssh gerrit gerrit query --format=JSON --all-approvals --comments -- -age:30d OR status:open > json
this finishes. For you this hangs?
#11 Updated by Teemu Murtola over 3 years ago
Roland Schulz wrote:
In the gerrit log I find:
[...]If I execute:
[...]
this finishes. For you this hangs?
The error is probably from me forcibly killing the process when it did not terminate. For me, it takes forever. I solved the issue by implementing pagination and doing the query in patches of 25 or 50 changes. It still takes really long, but at least it finishes. Some of the 25-change batches from 2016 took over 1 minute to retrieve with that query for me...
#12 Updated by Teemu Murtola over 3 years ago
Your queries still seem to have issues: the last two should probably do value!=0 instead of value=0. I tried pasting the statistics from my script (found at https://github.com/teemumurtola/gerrit-scripts), but for some reason Redmine gives internal server errors if I try that...
- These only count activity on public changes (not drafts), since drafts cannot be accessed by everyone (so the script cannot find them, either).
- My numbers include also releng changes.
- The number of comments is much smaller because I filter out some of the technical actions (like rebasing someone else's change). I'm not sure what other differences there are, as these can also come from differences in how
gerrit queryinterprets the SQL data vs. the raw query. - Roland's queries might miss the last day of the date range.
I haven't verified my scripts very carefully, so there can be also differences because of mis-interpretation of the data that gerrit query returns.
#13 Updated by Teemu Murtola over 3 years ago
Top Reviewers of the year by comments: Mark Abraham 655 Teemu Murtola 636 Berk Hess 531 David van der Spoel 275 Szilárd Páll 263 Erik Lindahl 239 Roland Schulz 168 Aleksei Iupinov 88 Michael Shirts 48 Magnus Lundborg 47
#14 Updated by Teemu Murtola over 3 years ago
Top Reviewers of the month by comments: Roland Schulz 39 Teemu Murtola 30 Mark Abraham 29 Berk Hess 27 Aleksei Iupinov 21 Magnus Lundborg 12 Erik Lindahl 8 Eric Irrgang 6 Vedran Miletic 4 Szilárd Páll 3
#15 Updated by Teemu Murtola over 3 years ago
Top Reviewers of the year by voted commits: Mark Abraham 236 Erik Lindahl 101 Teemu Murtola 88 David van der Spoel 87 Berk Hess 80 Aleksei Iupinov 61 Szilárd Páll 55 Roland Schulz 45 Magnus Lundborg 32 Carsten Kutzner 13 Top Reviewers of the month by voted commits: Aleksei Iupinov 17 Mark Abraham 16 Roland Schulz 15 Magnus Lundborg 11 Berk Hess 9 Teemu Murtola 9 Vedran Miletic 4 Szilárd Páll 3 Erik Lindahl 2 Eric Irrgang 2
Apparently pasting the c with accent in Vedran's name breaks Redmine...
#16 Updated by Roland Schulz over 3 years ago
I fixed the date and value bug Teemu pointed out. Teemu's solution is clearly better so I won't fix any of the other differences.
#17 Updated by Roland Schulz over 3 years ago
Teemu's script has a lot of more nice statistics then the once he has pasted so far. All of them are:
Number of open changes by owner and status ========================================== Name Open RFC/WIP -Verified -Review Approved +Review Comments Nothing ====================== ==== ======= ========= ======= ======== ======= ======== ======= Mark Abraham 54 5 26 7 1 6 7 2 Roland Schulz 24 12 5 1 0 1 4 1 Berk Hess 22 2 6 5 2 5 1 2 Aleksei Iupinov 10 2 0 0 2 1 4 1 Szilárd Páll 10 6 3 1 0 0 0 0 Teemu Murtola 9 2 0 0 4 1 1 1 Alexey Shvetsov 6 2 1 0 0 0 3 0 John Eblen 6 3 2 0 0 0 1 0 Viveca Lindahl 5 0 3 0 0 0 2 0 David van der Spoel 5 1 2 0 0 0 2 0 Vedran Miletic 4 0 3 0 0 1 0 0 Erik Lindahl 3 0 0 1 0 1 1 0 Thomas Ullmann 3 0 1 0 0 0 2 0 Christian Blau 3 1 1 0 0 0 1 0 Michael Shirts 2 0 1 0 0 0 0 1 Jan Henning Peters 2 0 2 0 0 0 0 0 Manuel Luitz 1 0 0 0 0 0 1 0 Rossen Apostolov 1 0 0 0 0 0 1 0 Jochen Hub 1 0 1 0 0 0 0 0 Carsten Kutzner 1 0 0 0 1 0 0 0 Alfredo Metere 1 1 0 0 0 0 0 0 Nina Fischer 1 1 0 0 0 0 0 0 Mohammad Ghahremanpour 1 0 1 0 0 0 0 0 James "Wes" Barnett 1 0 0 0 0 0 1 0 Grzegorz Wieczorek 1 0 1 0 0 0 0 0 Magnus Lundborg 1 0 0 0 0 0 1 0 Nicolae Goga 1 0 1 0 0 0 0 0 Christoph Junghans 1 0 0 1 0 0 0 0 Marcello Sega 1 0 1 0 0 0 0 0 Tomáš Trnka 1 0 0 1 0 0 0 0 Activity on open changes ======================== Name Commented Voted ========================== ========= ===== Teemu Murtola 71 23 Mark Abraham 47 11 Roland Schulz 44 13 Berk Hess 41 13 Erik Lindahl 32 14 David van der Spoel 32 14 Szilárd Páll 31 9 Aleksei Iupinov 12 10 Magnus Lundborg 10 10 Carsten Kutzner 9 6 Alexey Shvetsov 5 3 Rossen Apostolov 5 2 John Eblen 5 0 Kasson 5 3 Thomas Ullmann 4 3 Christoph Junghans 4 1 Michael Shirts 3 2 Christian Blau 2 2 Vedran Miletic 2 1 Justin Lemkul 2 1 Jochen Hub 2 2 James "Wes" Barnett 2 0 Manuel Luitz 1 0 Anders Gabrielsson 1 0 Unknown 1 0 Erik Marklund 1 1 Alex Björling 1 0 Maxim Koltsov 1 0 M. Eric Irrgang 1 1 Aleksei Iupinov (inactive) 1 0 Viveca Lindahl 1 0 Stefan Fleischmann 1 0 Jan Henning Peters 0 1 Number of changes during date range =================================== Name Created Merged Abandoned Both Commented Voted =================== ======= ====== ========= ==== ========= ===== Aleksei Iupinov 5 3 1 3 18 18 Roland Schulz 3 2 0 1 21 15 Mark Abraham 17 17 0 12 17 14 Berk Hess 5 2 1 2 19 12 Magnus Lundborg 0 0 0 0 10 10 Teemu Murtola 7 7 0 6 15 9 Vedran Miletic 2 0 2 1 4 4 David van der Spoel 0 1 0 0 3 3 Szilárd Páll 1 1 0 0 3 3 Erik Lindahl 0 0 0 0 3 2 Eric Irrgang 0 0 0 0 3 2 Carsten Kutzner 0 0 0 0 1 1 Christoph Junghans 0 0 0 0 1 1 James "Wes" Barnett 0 0 0 0 1 0 M. Eric Irrgang 1 0 1 1 0 0 Activity during date range ========================== Name Comments Technical Votes =================== ======== ========= ===== Roland Schulz 37 16 24 Berk Hess 32 4 22 Mark Abraham 27 14 23 Teemu Murtola 26 2 11 Aleksei Iupinov 21 0 25 Magnus Lundborg 10 1 15 Erik Lindahl 8 1 3 Eric Irrgang 5 0 5 Vedran Miletic 4 0 6 David van der Spoel 3 3 3 Szilárd Páll 3 3 5 Carsten Kutzner 1 0 1 James "Wes" Barnett 1 0 0 Christoph Junghans 1 0 2