Re: Commit reviews' author statistics: bus factor issue?
Den fre 23 apr. 2021 kl 12:05 skrev Luke Perkins : > Please remove this person from the mailing list. > If you have access to the e-mail address, I think it is easiest if you unsubscribe yourself. Instructions for the different mailing lists can be found at http://subversion.apache.org/mailing-lists.html For this particular mailing list: *Unsubscribe:* Send an email to dev-unsubscr...@subversion.apache.org Kind regards, Daniel Sahlberg
RE: Commit reviews' author statistics: bus factor issue?
Please remove this person from the mailing list. He has cancer is no longer
able to make cognitive decisions regarding this discussion.
Thank-you,
Caretaker of Luke Perkins
-Original Message-
From: Daniel Shahaf
Sent: Thursday, April 22, 2021 5:11 PM
To: Johan Corveleyn
Cc: Subversion Development
Subject: Re: Commit reviews' author statistics: bus factor issue?
Johan Corveleyn wrote on Thu, 22 Apr 2021 19:53 +00:00:
> On Thu, Apr 22, 2021 at 9:22 PM Daniel Shahaf wrote:
> >
> > [ Forwarding from private@ with an addition between triple dashes
> > and some paragraphs omitted altogether. ]
> >
> > Methodology: In my dev@ mailbox, I looked at "Re: svn commit"
> > threads where the subject line contained "trunk" somewhere, filtered
> > by date (using, e.g., ~s 'Re: svn commit' !~<( ~s 'Re: svn commit' ) ~d
> > '<730d'
> > ~s trunk in Mutt ). I then did a author histogram (the moral
> > equivalent of SELECT author, COUNT(*) AS cnt FROM
> > results_of_the_filter GROUP BY author ORDER BY cnt ).
> >
> > With the date filter set to ">6 years ago", the histogram is:
> > .
> > 1, 1, 1, 1, 2, 3, 6, 7, 10, 12, 13, 13, 19, 27, 49, 58, 86 .
> > Top three: 28.1%, 19.0%, 16.0%.
> >
> > With the date filter set to "<2 years ago", the histogram is:
> > .
> > 1, 1, 1, 1, 1, 1, 1, 1, 4, 5, 30 .
> > Top three: 64%, 10.6%, 8.5%.
> >
> > Do we have a bus factor problem?
> >
> > ---
> >
> > I'm deliberately not posting the author identities part of the
> > histograms. It's public info (and I literally did just post
> > instructions for how to compute it, for reproducibility), but no
> > individual's contributions or contribution statistics are the point.
> >
> > The histogram is of the authors of commit review threads, not of
> > everyone who participated in such threads.
> >
> > ---
> >
> > Having few reviewers is problematic in various ways:
> >
> > - Bus factor
> >
> > - Single point of failure (cf. Linus' Law)
> >
> > - Possibility of zero reviews for some areas of the code
> >
> > - Review standards should be seen as community standards rather than
> > a reviewer's idiosyncrasies; cf. the point about new projects needing
> > at least two mentors ("parents"), rather than just one
> >
> > - [not an exhaustive list]
> >
> > Cheers,
> >
> > Daniel
> >
> > There may be a better way to express "first in a thread". I tried
> > !~<(^) , but couldn't get it to work.
>
> Good point. But I believe we have many other areas where our bus
> factor is getting very low.
>
> - RM -- [ ]
>
> - Security issues: [ ]
>
> - Approving backports. [ ]
>
> - Signing releases: [ ]
>
>
> Perhaps the lack of review-activity for us as a CTR project is more
> critical, I don't know. Good observation in any case.
Note some of these issues are related: Approving backports amounts to reviewing
some specific commits carefully; work on security issues involves some
reviewing, some RMing, and some signing.
I'd be hesitant to say "more critical" because I'm not sure the criticalities
of these five areas are orderable, but historically, I think we rely on commit
reviews to catch certain classes of bugs: for instance, cross-version
compatibility (in the ABI, over the wire, in on-disk formats) and FSFS
concurrency guarantees are both core promises that have little test coverage.
And, of course, there's any number of C gotchas and portability quirks that our
compilers don't warn on (in the default build configuration).
Cheers,
Daniel
Re: Commit reviews' author statistics: bus factor issue?
Nathan Hartman wrote on Thu, 22 Apr 2021 21:41 +00:00: > Not knowing whether / how many people have reviewed a particular > commit is, as was said elsewhere, a silent failure mode of the CTR > (commit-then-review) convention. > > Do we want to try switching to a RTC (review-then-commit) convention? If we do switch to RTC, we might want to also retroactively ensure all commits post 1.14.x's branching have been reviewed by at least two pairs of eyes each. However, I wonder whether there's a smaller change we can do first, rather than a full-blown s/CTR/RTC/ flag day. For instance, how about we agree, for the next N weeks, to make our commit reviews explicit? I.e., to explicitly say "I've reviewed this commit and found no issues"? (Call this Commit-Then-Explicit-Review.) At the end of the N weeks, we can look at the experience and data we'll have then, and decide how to continue: whether to revert to CTR, switch to RTC, keep the new system and implement supporting tooling, etc.. > Some committers are already posting patches for review before > committing them. Another project I work on adopted the convention that > no one commits their own patches. It seems to work well there. Yes, > mistakes still get through sometimes, but at least we know that each > patch has had at least two pairs of eyes on it: the author's and the > committer's. OpenBSD uses a two-man rule too (or at least did, when I last checked), so stsp may have useful input here. > In our case, I don't think that convention should be > applied everywhere. Branches, the site, documentation, areas outside > the core code, don't need this extra step -- though in the case of > r1888589 the code in question is part of tests. That doesn't mean it's > less important. We do rely on the test suite and buildbots to alert us > to problems. Cheers, Daniel
CFP for ApacheCon 2021 closes in ONE WEEK
[You are receiving this because you're subscribed to one or more dev@ mailing lists for an Apache project, or the ApacheCon Announce list.] Time is running out to submit your talk for ApacheCon 2021. The Call for Presentations for ApacheCon @Home 2021, focused on Europe and North America time zones, closes May 3rd, and is at https://www.apachecon.com/acah2021/cfp.html The CFP for ApacheCon Asia, focused on Asia/Pacific time zones, is at https://apachecon.com/acasia2021/cfp.html and also closes on May 3rd. ApacheCon is our main event, featuring content from any and all of our projects, and is your best opportunity to get your project in front of the largest audience of enthusiasts. Please don't wait for the last minute. Get your talks in today! -- Rich Bowen, VP Conferences The Apache Software Foundation https://apachecon.com/ @apachecon
Re: [PATCH] svn_test_main: Extra safety net for unthreaded builds
On Wed, Apr 21, 2021 at 9:58 PM Daniel Shahaf wrote:
>
> svn_test_main.c:1073 has a condition of the form:
>
> /* just run all tests */
> ⋮
> if (max_threads == 1 || !parallel)
> {
> ⋮
> }
> #if APR_HAS_THREADS
> else
> {
> ⋮
> }
> #endif
>
> max_threads is a value hard-coded in each individual foo-test.c file.
>
> «parallel» should always be FALSE in unthreaded builds, but if for some
> reason it were TRUE, the foo-test executable would silently exit 0
> without running any tests, so how about this? —
Agreed that the way it's written now leaves open that possibility if
something (else) should go wrong.
(snip inline patch)
> Seems safe enough, but I don't have a threadless APR build handy to test
> this with.
LGTM.
I couldn't apply the patch as copy-pasted from your email so I
recreated the change manually (patch of my manual change attached as
.txt for reference).
I just finished building and running the test suite against trunk @
r1889114 with this patch and threadless (specifically with
/tools/dev/unix-build with 'make THREADING=no') and there were no
failures or any problems I could see.
I verified APR's configure's output contained:
Checking for Threads...
APR will be non-threaded
Should be safe to commit.
Cheers,
Nathan
Index: subversion/tests/svn_test_main.c
===
--- subversion/tests/svn_test_main.c(revision 1889130)
+++ subversion/tests/svn_test_main.c(working copy)
@@ -1089,9 +1089,9 @@
svn_pool_clear(cleanup_pool);
}
}
-#if APR_HAS_THREADS
else
{
+#if APR_HAS_THREADS
got_error = do_tests_concurrently(opts.prog_name, test_funcs,
array_size, max_threads,
&opts, test_pool);
@@ -1099,8 +1099,11 @@
/* Execute all cleanups */
svn_pool_clear(test_pool);
svn_pool_clear(cleanup_pool);
+#else
+ /* Can't happen */
+ SVN_ERR_MALFUNCTION();
+#endif
}
-#endif
}
/* Clean up APR */
Re: [PATCH] svn_test_main: Extra safety net for unthreaded builds
Nathan Hartman wrote on Fri, 23 Apr 2021 17:40 +00:00: > I couldn't apply the patch as copy-pasted from your email so I > recreated the change manually (patch of my manual change attached as > .txt for reference). The patch in the archives should apply cleanly, as far as I can tell. > Should be safe to commit. Thanks a lot for reviewing and testing! Committed as r1889133.
