Would it make sense to allow people submitting the patch to flag things as LHF or small tasks? If it doesn't look like it is simple enough the team could remove the label but it may help get feedback to patches more quickly, even something saying accepted for review would be nice.
Personally if a ticket sits for a few weeks I have no idea what next steps I should take to keep it moving forward. We may want to document what a person submitting a patch should do next and if it is documented we should add a link on http://cassandra.apache.org/doc/latest/development/patches.html Jeff PS: My example is https://issues.apache.org/jira/browse/CASSANDRA-12633 On Wed, Oct 19, 2016 at 12:21 PM Edward Capriolo <edlinuxg...@gmail.com> wrote: > Also no one has said anything to the effect of 'we want to rubber stamp > reviews' so that ...evil reason. Many of us are coders by trade and > understand why that is bad. > > On Wednesday, October 19, 2016, Edward Capriolo <edlinuxg...@gmail.com> > wrote: > > > 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 > > <javascript:_e(%7B%7D,'cvml','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> > 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=08AGY6txKs > >> vMOP6lYkHQpPMRA1U6kqhAwGa8-0QCg3M&r=yfYEBHVkX6l0zImlOIBID > >> 0gmhluYPD5Jje-3CtaT3ow&m=eXI0TPp0DM06kmTuJQRNcUX5zy_O_KhWcDK > >> MA-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=08AGY6txKs > >> vMOP6lYkHQpPMRA1U6kqhAwGa8-0QCg3M&r=yfYEBHVkX6l0zImlOIBID > >> 0gmhluYPD5Jje-3CtaT3ow&m=eXI0TPp0DM06kmTuJQRNcUX5zy_O_KhWcDK > >> MA-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. > > > > > -- > Sorry this was sent from mobile. Will do less grammar and spell check than > usual. >
