Re: Low hanging fruit crew

[Nate McCall]

Thanks Jeremy. To come back to the original idea, for an LHF crew how
about as these:
- a weekly(?) list of LHF tickets sent to dev list
- sharing fliters of the above (can we put these on the how to contribute page?)
- adopt Jeff B.'s idea of tagging LHF your own self as part of our
culture/process
- ping on a LHF Patch Ava. ticket if its been reviewed and just hanging out

Extra credit:
- ping on a PA ticket (dev list even) if its not been reviewed, but
you think it's valuable/critical and it's past your review comfort
level



On Thu, Oct 20, 2016 at 11:39 AM, Jeremy Hanna
 wrote:
> It sounds like everyone is on the same page - there won’t be a rubber stamp.  
> It’s just to have a concerted effort to help these tickets move along as 
> they’re often the first experience that contributors have with the project.  
> I’m sure many of you know of the ‘lhf' tag that’s out there with an 
> associated Jira search [1] on the How to Contribute page [2].  In the Jira 
> search, you can see several that are either “patch available” or “awaiting 
> feedback” so that search a place to start.  As for something fancier like a 
> dashboard, I currently can’t make a share-able dashboard as a contributor to 
> the project.  Perhaps committers have more permissions there.
>
> 1) 
> https://issues.apache.org/jira/issues/?jql=project%20%3D%2012310865%20AND%20labels%20%3D%20lhf%20AND%20status%20!%3D%20resolved
> 2) https://wiki.apache.org/cassandra/HowToContribute
>
>
>
>> On Oct 19, 2016, at 5:27 PM, Jonathan Haddad  wrote:
>>
>> 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  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 
>>> 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 
 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  > 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" 
 wrote:
>>
>>> And just to be clear, I think everyone would welcome more testing for
>> both regressions of new 

Re: Low hanging fruit crew

[Jeremy Hanna]

It sounds like everyone is on the same page - there won’t be a rubber stamp.  
It’s just to have a concerted effort to help these tickets move along as 
they’re often the first experience that contributors have with the project.  
I’m sure many of you know of the ‘lhf' tag that’s out there with an associated 
Jira search [1] on the How to Contribute page [2].  In the Jira search, you can 
see several that are either “patch available” or “awaiting feedback” so that 
search a place to start.  As for something fancier like a dashboard, I 
currently can’t make a share-able dashboard as a contributor to the project.  
Perhaps committers have more permissions there.

1) 
https://issues.apache.org/jira/issues/?jql=project%20%3D%2012310865%20AND%20labels%20%3D%20lhf%20AND%20status%20!%3D%20resolved
2) https://wiki.apache.org/cassandra/HowToContribute



> On Oct 19, 2016, at 5:27 PM, Jonathan Haddad  wrote:
> 
> 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  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 
>> 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 
>>> 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 > 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" 
>>> 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 
> wrote:
>>> 
>>> I specifically used the phrase "problems that the test would not"
>> to
> show I
>>> am talking about more than 

Re: Low hanging fruit crew

[Jonathan Haddad]

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  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 
> 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 
> > 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  > > > 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" 
> > 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 
> > >> 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

Re: Low hanging fruit crew

[Jeff Beck]

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 
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 
> 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  > > 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" 
> 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 
> >> 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, 

Re: Low hanging fruit crew

[Edward Capriolo]

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 
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"  > 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  > 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  > wrote:
> >>
> >>> Shouldn't the tests test the code for correctness?
> >>>
> >>> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis  > 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.
> 

Re: Low hanging fruit crew

[Jeff Jirsa]

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"  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  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  wrote:
>> 
>>> Shouldn't the tests test the code for correctness?
>>> 
>>> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis  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=DQIFaQ=08AGY6txKsvMOP6lYkHQpPMRA1U6kqhAwGa8-0QCg3M=yfYEBHVkX6l0zImlOIBID0gmhluYPD5Jje-3CtaT3ow=eXI0TPp0DM06kmTuJQRNcUX5zy_O_KhWcDKMA-jxww0=D28xk3JpCIOAQnCXJAVky0lJJv_mPnYwy4gKxLKsSqw=
  
 @spyced
 
>>> 
>> 
>> 
>> 
>> -- 
>> Jonathan Ellis
>> co-founder, 
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__www.datastax.com=DQIFaQ=08AGY6txKsvMOP6lYkHQpPMRA1U6kqhAwGa8-0QCg3M=yfYEBHVkX6l0zImlOIBID0gmhluYPD5Jje-3CtaT3ow=eXI0TPp0DM06kmTuJQRNcUX5zy_O_KhWcDKMA-jxww0=D28xk3JpCIOAQnCXJAVky0lJJv_mPnYwy4gKxLKsSqw=
>>  
>> @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.

Re: Low hanging fruit crew

[Jeremy Hanna]

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  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  wrote:
> 
>> Shouldn't the tests test the code for correctness?
>> 
>> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis  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, http://www.datastax.com
>>> @spyced
>>> 
>> 
> 
> 
> 
> -- 
> Jonathan Ellis
> co-founder, http://www.datastax.com
> @spyced

Re: Low hanging fruit crew

[Jonathan Ellis]

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  wrote:

> Shouldn't the tests test the code for correctness?
>
> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis  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, http://www.datastax.com
> > @spyced
> >
>



-- 
Jonathan Ellis
co-founder, http://www.datastax.com
@spyced

Re: Low hanging fruit crew

[Jeremiah D Jordan]

Unless the reviewer reviews the for content, then you don’t know if they do or 
not.

-Jeremiah

> On Oct 19, 2016, at 10:52 AM, Jonathan Haddad  wrote:
> 
> Shouldn't the tests test the code for correctness?
> 
> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis  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, http://www.datastax.com
>> @spyced
>> 

Re: Low hanging fruit crew

[Jonathan Haddad]

Shouldn't the tests test the code for correctness?

On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis  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, http://www.datastax.com
> @spyced
>

Re: Low hanging fruit crew

[Jonathan Ellis]

On Wed, Oct 19, 2016 at 8:27 AM, Benjamin Lerer  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, http://www.datastax.com
@spyced

Re: Low hanging fruit crew

[Benjamin Lerer]

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.


On Wed, Oct 19, 2016 at 3:01 PM, Edward Capriolo 
wrote:

> Yes. The LHFC crew should always pay it forward. Not many of us have a
> super computer to run all the tests, but for things that are out there
> marked patch_available apply it to see that it applies clean, if it
> includes a test run that test (and possibly some related ones in the
> file/folder etc for quick coverage). A nice initial sweep is a good thing.
>
> I have seen before a process which triggered and auto-build when the patch
> was added to the ticket. This took a burden off the committers, by the time
> someone got to the ticket they already knew if tests passed then it was
> only a style and fine tuning review.
>
> In this case if we have a good initial pass we can hopefully speed along
> the process.
>
> On Wed, Oct 19, 2016 at 4:18 AM, kurt Greaves 
> wrote:
>
> > On 19 October 2016 at 05:30, Nate McCall  wrote:
> >
> > > if you are offering up resources for review and test coverage,
> > > there is work out there. Most immediately in the department of reviews
> > > for "Patch Available."
> > >
> >
> > We can certainly put some minds to this. There are a few of us with a
> very
> > good understanding of working Cassandra yet could use more exposure to
> the
> > code base. We'll start getting out there and looking for things to
> review.
> >
> > Kurt Greaves
> > k...@instaclustr.com
> > www.instaclustr.com
> >
>

Re: Low hanging fruit crew

[Edward Capriolo]

Yes. The LHFC crew should always pay it forward. Not many of us have a
super computer to run all the tests, but for things that are out there
marked patch_available apply it to see that it applies clean, if it
includes a test run that test (and possibly some related ones in the
file/folder etc for quick coverage). A nice initial sweep is a good thing.

I have seen before a process which triggered and auto-build when the patch
was added to the ticket. This took a burden off the committers, by the time
someone got to the ticket they already knew if tests passed then it was
only a style and fine tuning review.

In this case if we have a good initial pass we can hopefully speed along
the process.

On Wed, Oct 19, 2016 at 4:18 AM, kurt Greaves  wrote:

> On 19 October 2016 at 05:30, Nate McCall  wrote:
>
> > if you are offering up resources for review and test coverage,
> > there is work out there. Most immediately in the department of reviews
> > for "Patch Available."
> >
>
> We can certainly put some minds to this. There are a few of us with a very
> good understanding of working Cassandra yet could use more exposure to the
> code base. We'll start getting out there and looking for things to review.
>
> Kurt Greaves
> k...@instaclustr.com
> www.instaclustr.com
>

Re: Low hanging fruit crew

[kurt Greaves]

On 19 October 2016 at 05:30, Nate McCall  wrote:

> if you are offering up resources for review and test coverage,
> there is work out there. Most immediately in the department of reviews
> for "Patch Available."
>

We can certainly put some minds to this. There are a few of us with a very
good understanding of working Cassandra yet could use more exposure to the
code base. We'll start getting out there and looking for things to review.

Kurt Greaves
k...@instaclustr.com
www.instaclustr.com

Re: Low hanging fruit crew

[Nate McCall]

That's a good idea, Ed, thanks for bringing this up.

Kurt, if you are offering up resources for review and test coverage,
there is work out there. Most immediately in the department of reviews
for "Patch Available."

While not quite low-hanging fruit, it's helpful to have non-committers
look through the patch while following the review checklist [0] and
then running test targets to make sure there are no (covered at least)
regressions.

Non-committers can non-binding +1 a patch and that makes it easier to
just commit as is. Obviously there is some requirements for knowing
what you are looking at and being comfortable with reviewing the
complexity of the patch.

Also, we as committers could do a better job (myself at the top of the
list here) of quickly scanning said +1'ed patches where reviewer has
run the tests, and then committing them.

We'll probably have a bug introduced here and there, but the tradeoff
is bringing more people up to speed and broadening the active
day-to-day pool of resources and eventually adding committers.

Thoughts?

[0] http://cassandra.apache.org/doc/latest/development/how_to_review.html

On Wed, Oct 19, 2016 at 11:58 AM, kurt Greaves  wrote:
> So there are a bunch of us at Instaclustr looking into contributing to the
> project on a more frequent basis. We'd definitely be interested in some
> kind of LHF crew under whom our new "contributors" could make tracks on
> becoming main committers, while having some close by colleagues who can
> help them through. Having said that I'm happy to be part of the so called
> LHF crew as soon as I'm qualified enough to do so. I'm currently full time
> on contributing to the C* project, so hopefuly it shouldn't be too long
> before I'm able to take on some of these duties.
>
> Kurt Greaves
> k...@instaclustr.com
> www.instaclustr.com

Re: Low hanging fruit crew

[kurt Greaves]

So there are a bunch of us at Instaclustr looking into contributing to the
project on a more frequent basis. We'd definitely be interested in some
kind of LHF crew under whom our new "contributors" could make tracks on
becoming main committers, while having some close by colleagues who can
help them through. Having said that I'm happy to be part of the so called
LHF crew as soon as I'm qualified enough to do so. I'm currently full time
on contributing to the C* project, so hopefuly it shouldn't be too long
before I'm able to take on some of these duties.

Kurt Greaves
k...@instaclustr.com
www.instaclustr.com

Low hanging fruit crew

[Edward Capriolo]

I go through the Cassandra jira weekly and I notice a number of tickets
which appear to be clear issues or requests for simple metrics.

https://issues.apache.org/jira/browse/CASSANDRA-12626

https://issues.apache.org/jira/browse/CASSANDRA-12330

I also have a few jira issues (opinion) would be simple to triage and
merge. Getting things merged is the primary pathway to meritocracy in the
ASF.

Across some other ASF projects I have seen that when the number of small
patches becomes larger that bodies to review them. It can result and chick
and egg scenario where reviewers feel overburdened, but the pathway to
removing this burden is promoting contributors to committers.

My suggestion:
Assemble a low-hanging-fruit-crew. This crew would be armed general support
for small commits, logging, metrics, test coverage, things static analysis
reveals etc. They would have a reasonable goal like "get 2 lhf merged a
day/week/whatever". If the process is successful, in a few months there
would hopefully be 1-2 committers graduated who would naturally wish to
move into low hanging fruit duties.

Thoughts?