I realize that test passing a small tests and trivial reviews will not catch all issues. I am not attempting to trivialize the review process.
Both deep and shallow bugs exist. The deep bugs, I am not convinced that even an expert looking at the contribution for N days can account for a majority of them. The shallow ones may appear shallow and may be deep, but given that a bug already exists an attempt to fix it usually does not arrive at something worse. Many of these issues boil down to simple, seeemingly clear fixes. Some are just basic metric addition. Many have no interaction for weeks. On Wednesday, October 19, 2016, Jeff Jirsa <jeff.ji...@crowdstrike.com> wrote: > Let’s not get too far in the theoretical weeds. The email thread really > focused on low hanging tickets – tickets that need review, but definitely > not 8099 level reviews: > > 1) There’s a lot of low hanging tickets that would benefit from outside > contributors as their first-patch in Cassandra (like > https://issues.apache.org/jira/browse/CASSANDRA-12541 , > https://issues.apache.org/jira/browse/CASSANDRA-12776 ) > 2) Some of these patches already exist and just need to be reviewed and > eventually committed. > > Folks like Ed and Kurt have been really active in Jira lately, and there > aren’t a ton of people currently reviewing/committing – that’s part of OSS > life, but the less friction that exists getting those patches reviewed and > committed, the more people will be willing to contribute in the future, and > the better off the project will be. > > > On 10/19/16, 9:14 AM, "Jeremy Hanna" <jeremy.hanna1...@gmail.com > <javascript:;>> wrote: > > >And just to be clear, I think everyone would welcome more testing for > both regressions of new code correctness. I think everyone would > appreciate the time savings around more automation. That should give more > time for a thoughtful review - which is likely what new contributors really > need to get familiar with different parts of the codebase. These LHF > reviews won’t be like the code reviews of the vnode or the 8099 ticket > obviously, so it won’t be a huge burden. But it has some very tangible > benefits imo, as has been said. > > > >> On Oct 19, 2016, at 11:08 AM, Jonathan Ellis <jbel...@gmail.com > <javascript:;>> wrote: > >> > >> I specifically used the phrase "problems that the test would not" to > show I > >> am talking about more than mechanical correctness. Even if the tests > are > >> perfect (and as Jeremiah points out, how will you know that without > reading > >> the code?), you can still pass tests with bad code. And is expecting > >> perfect tests really realistic for multithreaded code? > >> > >> But besides correctness, reviews should deal with > >> > >> 1. Efficiency. Is there a quadratic loop that will blow up when the > number > >> of nodes in a cluster gets large? Is there a linear amount of memory > used > >> that will cause problems when a partition gets large? These are not > >> theoretical problems. > >> > >> 2. Maintainability. Is this the simplest way to accomplish your > goals? Is > >> there a method in SSTableReader that would make your life easier if you > >> refactored it a bit instead of reinventing it? Does this reduce > technical > >> debt or add to it? > >> > >> 3. "Bus factor." If only the author understands the code and all anyone > >> else knows is that tests pass, the project will quickly be in bad shape. > >> Review should ensure that at least one other person understand the code, > >> what it does, and why, at a level that s/he could fix bugs later in it > if > >> necessary. > >> > >> On Wed, Oct 19, 2016 at 10:52 AM, Jonathan Haddad <j...@jonhaddad.com > <javascript:;>> wrote: > >> > >>> Shouldn't the tests test the code for correctness? > >>> > >>> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis <jbel...@gmail.com > <javascript:;>> wrote: > >>> > >>>> On Wed, Oct 19, 2016 at 8:27 AM, Benjamin Lerer < > >>>> benjamin.le...@datastax.com <javascript:;> > >>>>> wrote: > >>>> > >>>>> Having the test passing does not mean that a patch is fine. Which is > >>> why > >>>> we > >>>>> have a review check list. > >>>>> I never put a patch available without having the tests passing but > most > >>>> of > >>>>> my patches never pass on the first try. We always make mistakes no > >>> matter > >>>>> how hard we try. > >>>>> The reviewer job is to catch those mistakes by looking at the patch > >>> from > >>>>> another angle. Of course, sometime, both of them fail. > >>>>> > >>>> > >>>> Agreed. Review should not just be a "tests pass, +1" rubber stamp, > but > >>>> actually checking the code for correctness. The former is just > process; > >>>> the latter actually catches problems that the tests would not. (And > this > >>>> is true even if the tests are much much better than ours.) > >>>> > >>>> -- > >>>> Jonathan Ellis > >>>> co-founder, https://urldefense.proofpoint.com/v2/url?u=http-3A__www. > datastax.com&d=DQIFaQ&c=08AGY6txKsvMOP6lYkHQpPMRA1U6kqhAwGa8-0QCg3M&r= > yfYEBHVkX6l0zImlOIBID0gmhluYPD5Jje-3CtaT3ow&m= > eXI0TPp0DM06kmTuJQRNcUX5zy_O_KhWcDKMA-jxww0&s=D28xk3JpCIOAQnCXJAVky0lJJv_ > mPnYwy4gKxLKsSqw&e= > >>>> @spyced > >>>> > >>> > >> > >> > >> > >> -- > >> Jonathan Ellis > >> co-founder, https://urldefense.proofpoint.com/v2/url?u=http-3A__www. > datastax.com&d=DQIFaQ&c=08AGY6txKsvMOP6lYkHQpPMRA1U6kqhAwGa8-0QCg3M&r= > yfYEBHVkX6l0zImlOIBID0gmhluYPD5Jje-3CtaT3ow&m= > eXI0TPp0DM06kmTuJQRNcUX5zy_O_KhWcDKMA-jxww0&s=D28xk3JpCIOAQnCXJAVky0lJJv_ > mPnYwy4gKxLKsSqw&e= > >> @spyced > > > > ____________________________________________________________________ > CONFIDENTIALITY NOTE: This e-mail and any attachments are confidential and > may be legally privileged. If you are not the intended recipient, do not > disclose, copy, distribute, or use this email or any attachments. If you > have received this in error please let the sender know and then delete the > email and all attachments. > -- Sorry this was sent from mobile. Will do less grammar and spell check than usual.
