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> 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> 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> wrote: >> >>> Shouldn't the tests test the code for correctness? >>> >>> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis <jbel...@gmail.com> wrote: >>> >>>> On Wed, Oct 19, 2016 at 8:27 AM, Benjamin Lerer < >>>> benjamin.le...@datastax.com >>>>> 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.
smime.p7s
Description: S/MIME cryptographic signature
