Tagging tickets as LHF is a great idea. There's plenty of people that would love to set up a JIRA dashboard saved search / nightly email for LHF tickets.
On Wed, Oct 19, 2016 at 1:34 PM Jeff Beck <beckj...@gmail.com> wrote: > 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. > > >
