Project

General

Profile

Task #2089

Encourage code review

Added by Roland Schulz about 3 years ago. Updated almost 3 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Difficulty:
uncategorized
Close

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 almost 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 almost 3 years ago

How about "code-reviewer of the month" awards?

#3 Updated by Szilárd Páll almost 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 almost 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 almost 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 almost 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 almost 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 almost 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 almost 3 years ago

Thanks! Of course you are right. I update the queries and the table.

#10 Updated by Roland Schulz almost 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 almost 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 almost 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...

The numbers are somewhat different because of several reasons:
  • 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 query interprets 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 almost 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 almost 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 almost 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 almost 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 almost 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  

Also available in: Atom PDF