Re: Code Reviews

[Adrien Grand <jp...@gmail.com>]

Like Dawid I hope we won't add strict requirements to get changes reviewed
before merging but I do agree with the general sentiment that reviews are
helpful and improve code quality. I really appreciate getting feedback on
patches that I upload, including negative feedback and I don't mind being
pinged on issues if anyone thinks I might have valuable feedback to give.

I didn't know Solr had a CTR policy. I understand CTR and RTC have pros and
cons but since there seems to be agreement that we want more changes to be
reviewed I think RTC is better at encouraging a review culture: as a
reviewer it's easier to recommend that the change should be done in a
totally different way if that is what you think, and you also feel more
useful since someone considered that the change needs your pair of eyes
before being merged.

Le mer. 28 févr. 2018 à 21:07, Cassandra Targett <ca...@gmail.com> a
écrit :

> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey <ap...@elyograg.org> wrote:
>
>>
>> I notice in ZK issues that projects associated with Hadoop have an
>> *automatic* machine-generated QA check whenever a patch is submitted on
>> those projects.  This obviously is not the same as a real review by a
>> person, but the info it outputs seems useful.
>>
>>
>>
> This is what SOLR-10912 intends to achieve.
>
>

Re: Code Reviews

[Varun Thacker <va...@vthacker.in>]

+1 to the general sentiment of trying to get more eyes on a larger change
before committing.

We could start making a more conscious decision of calling out these
changes and then seeing if it's gets any attention

I don't have any tool preference so I started with review board for my
latest patch to see where it takes


On Thu, Mar 1, 2018 at 11:32 AM, Stefan Matheis <st...@mathe.is> wrote:

> > This seems to be what the majority thinks and I see the point, I’m
> concerned of this myself. I’m just not sure how to encourage people to
> submit for review and review other peoples more since the option is there
> now and is not very frequently used. I’m open to suggestions if anyone has
> ideas.
>
> It's not an easy thing to apply and the details are maybe a bit witty ..
> I've seen a budget/credit system in use. Where you earn points doing
> reviews .. which are basically needed to get something in _without_ a
> review.
>
> I'm not proposing exactly that, because I don't like the incentive it's
> using .. just the underlying system is appealing. Sometimes you need a
> little bit more than just say please.
>
> Perhaps picking up what Shawn mentioned: if you could "subscribe" to
> certain parts of the code and get notified about patches that are related
> to it? Obviously that's not a perfect solution by itself either (because it
> could potentially mean that people are only just looking at those anymore
> and none of the others) ..
>
> But it's what some git(hub)-based review systems use: they @-mention you
> in a upcoming change because you touched that code at least once before. So
> you are kind of a good fit to review it.
>
> ... Uhm okay, that was not as straight forward as I'd like my reply to be
> - hopefully it still helps :)
>
> Best
> Stefan
>
> On Mar 1, 2018 4:59 AM, "Tomas Fernandez Lobbe" <tf...@apple.com> wrote:
>
>
> Like Dawid I hope we won't add strict requirements to get changes reviewed
> before merging but I do agree with the general sentiment that reviews are
> helpful and improve code quality.
>
> This seems to be what the majority thinks and I see the point, I’m
> concerned of this myself. I’m just not sure how to encourage people to
> submit for review and review other peoples more since the option is there
> now and is not very frequently used. I’m open to suggestions if anyone has
> ideas.
>
> I really appreciate getting feedback on patches that I upload, including
> negative feedback and I don't mind being pinged on issues if anyone thinks
> I might have valuable feedback to give.
>
> Exactly, same here. The times I got my patches reviewed and I got very
> valuable feedback, including someone fixing something broken in my patch.
>
> I encourage people to go and review some random commits and see if they
> could have given any valuable feedback. Someone could tell me “you can go,
> review, and create a new Jira with your proposed changes”, but that doesn’t
> happen usually, so back to my point.
>
>
> On Feb 28, 2018, at 5:11 PM, Adrien Grand <jp...@gmail.com> wrote:
>
> Like Dawid I hope we won't add strict requirements to get changes reviewed
> before merging but I do agree with the general sentiment that reviews are
> helpful and improve code quality. I really appreciate getting feedback on
> patches that I upload, including negative feedback and I don't mind being
> pinged on issues if anyone thinks I might have valuable feedback to give.
>
> I didn't know Solr had a CTR policy. I understand CTR and RTC have pros
> and cons but since there seems to be agreement that we want more changes to
> be reviewed I think RTC is better at encouraging a review culture: as a
> reviewer it's easier to recommend that the change should be done in a
> totally different way if that is what you think, and you also feel more
> useful since someone considered that the change needs your pair of eyes
> before being merged.
>
> Le mer. 28 févr. 2018 à 21:07, Cassandra Targett <ca...@gmail.com>
> a écrit :
>
>> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey <ap...@elyograg.org>
>> wrote:
>>
>>>
>>> I notice in ZK issues that projects associated with Hadoop have an
>>> *automatic* machine-generated QA check whenever a patch is submitted on
>>> those projects.  This obviously is not the same as a real review by a
>>> person, but the info it outputs seems useful.
>>>
>>>
>>>
>> This is what SOLR-10912 intends to achieve.
>>
>>
>
>

Re: Code Reviews

[Stefan Matheis <st...@mathe.is>]

> This seems to be what the majority thinks and I see the point, I’m
concerned of this myself. I’m just not sure how to encourage people to
submit for review and review other peoples more since the option is there
now and is not very frequently used. I’m open to suggestions if anyone has
ideas.

It's not an easy thing to apply and the details are maybe a bit witty ..
I've seen a budget/credit system in use. Where you earn points doing
reviews .. which are basically needed to get something in _without_ a
review.

I'm not proposing exactly that, because I don't like the incentive it's
using .. just the underlying system is appealing. Sometimes you need a
little bit more than just say please.

Perhaps picking up what Shawn mentioned: if you could "subscribe" to
certain parts of the code and get notified about patches that are related
to it? Obviously that's not a perfect solution by itself either (because it
could potentially mean that people are only just looking at those anymore
and none of the others) ..

But it's what some git(hub)-based review systems use: they @-mention you in
a upcoming change because you touched that code at least once before. So
you are kind of a good fit to review it.

... Uhm okay, that was not as straight forward as I'd like my reply to be -
hopefully it still helps :)

Best
Stefan

On Mar 1, 2018 4:59 AM, "Tomas Fernandez Lobbe" <tf...@apple.com> wrote:


Like Dawid I hope we won't add strict requirements to get changes reviewed
before merging but I do agree with the general sentiment that reviews are
helpful and improve code quality.

This seems to be what the majority thinks and I see the point, I’m
concerned of this myself. I’m just not sure how to encourage people to
submit for review and review other peoples more since the option is there
now and is not very frequently used. I’m open to suggestions if anyone has
ideas.

I really appreciate getting feedback on patches that I upload, including
negative feedback and I don't mind being pinged on issues if anyone thinks
I might have valuable feedback to give.

Exactly, same here. The times I got my patches reviewed and I got very
valuable feedback, including someone fixing something broken in my patch.

I encourage people to go and review some random commits and see if they
could have given any valuable feedback. Someone could tell me “you can go,
review, and create a new Jira with your proposed changes”, but that doesn’t
happen usually, so back to my point.


On Feb 28, 2018, at 5:11 PM, Adrien Grand <jp...@gmail.com> wrote:

Like Dawid I hope we won't add strict requirements to get changes reviewed
before merging but I do agree with the general sentiment that reviews are
helpful and improve code quality. I really appreciate getting feedback on
patches that I upload, including negative feedback and I don't mind being
pinged on issues if anyone thinks I might have valuable feedback to give.

I didn't know Solr had a CTR policy. I understand CTR and RTC have pros and
cons but since there seems to be agreement that we want more changes to be
reviewed I think RTC is better at encouraging a review culture: as a
reviewer it's easier to recommend that the change should be done in a
totally different way if that is what you think, and you also feel more
useful since someone considered that the change needs your pair of eyes
before being merged.

Le mer. 28 févr. 2018 à 21:07, Cassandra Targett <ca...@gmail.com> a
écrit :

> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey <ap...@elyograg.org> wrote:
>
>>
>> I notice in ZK issues that projects associated with Hadoop have an
>> *automatic* machine-generated QA check whenever a patch is submitted on
>> those projects.  This obviously is not the same as a real review by a
>> person, but the info it outputs seems useful.
>>
>>
>>
> This is what SOLR-10912 intends to achieve.
>
>

Re: Code Reviews

[Tomas Fernandez Lobbe <tf...@apple.com>]

> Like Dawid I hope we won't add strict requirements to get changes reviewed before merging but I do agree with the general sentiment that reviews are helpful and improve code quality.
This seems to be what the majority thinks and I see the point, I’m concerned of this myself. I’m just not sure how to encourage people to submit for review and review other peoples more since the option is there now and is not very frequently used. I’m open to suggestions if anyone has ideas. 

> I really appreciate getting feedback on patches that I upload, including negative feedback and I don't mind being pinged on issues if anyone thinks I might have valuable feedback to give.
Exactly, same here. The times I got my patches reviewed and I got very valuable feedback, including someone fixing something broken in my patch.

I encourage people to go and review some random commits and see if they could have given any valuable feedback. Someone could tell me “you can go, review, and create a new Jira with your proposed changes”, but that doesn’t happen usually, so back to my point.


> On Feb 28, 2018, at 5:11 PM, Adrien Grand <jp...@gmail.com> wrote:
> 
> Like Dawid I hope we won't add strict requirements to get changes reviewed before merging but I do agree with the general sentiment that reviews are helpful and improve code quality. I really appreciate getting feedback on patches that I upload, including negative feedback and I don't mind being pinged on issues if anyone thinks I might have valuable feedback to give.
> 
> I didn't know Solr had a CTR policy. I understand CTR and RTC have pros and cons but since there seems to be agreement that we want more changes to be reviewed I think RTC is better at encouraging a review culture: as a reviewer it's easier to recommend that the change should be done in a totally different way if that is what you think, and you also feel more useful since someone considered that the change needs your pair of eyes before being merged.
> 
> Le mer. 28 févr. 2018 à 21:07, Cassandra Targett <casstargett@gmail.com <ma...@gmail.com>> a écrit :
> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey <apache@elyograg.org <ma...@elyograg.org>> wrote:
> 
> I notice in ZK issues that projects associated with Hadoop have an
> *automatic* machine-generated QA check whenever a patch is submitted on
> those projects.  This obviously is not the same as a real review by a
> person, but the info it outputs seems useful.
> 
> 
> 
> This is what SOLR-10912 intends to achieve. 
>