Re: Code Reviews

[Varun Thacker]

+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  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"  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  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 
> a écrit :
>
>> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey 
>> 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]

> 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"  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  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  a
écrit :

> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey  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]

> 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  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  > a écrit :
> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey  > 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

[Adrien Grand]

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  a
écrit :

> On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey  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

[Cassandra Targett]

On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey  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

[Shawn Heisey]

On 2/28/2018 10:59 AM, Tomas Fernandez Lobbe wrote:
> In an effort to improve code quality, I’d like to suggest that we
> start requiring code review to non-trivial patches. Not sure if/how
> other open source projects are doing code reviews, but I’ve been using
> it in internal projects for many years and it’s a great way to catch
> bugs early, some of them very difficult to catch in unit tests

I *want* people to review the changes I suggest before I commit them. 
When the change is non-trivial, or has a larger impact than the patch
size would suggest, I will typically explicitly ASK for it to be
reviewed in the issue comments.  Even in cases where I don't explicitly
ask, I will usually leave the issue alone after submitting a patch to
allow time for interested parties to comment.

Sometimes I get a review.  Often I don't.

On the other side of the coin, I try to keep tabs on issues where I have
an interest, or at least have enough knowledge to comment, and look into
any suggested changes to see if they look OK to me.  There is a LOT of
Jira activity though, and it's hard to keep up with it.  I suspect that
my fellow Solr committers are in much the same situation -- they don't
understand the entirety of the codebase well enough to comment on more
than a handful of issues, and they're overwhelmed by the volume.

I'm not opposed to something formal, but I do wonder whether it might
make people hesitant to even suggest a change, much less work on it and
make commits, because they're worried that the entire idea will get shot
down during a formal review.  Also, it would increase the number of
messages that I have to wade through on a daily basis, which won't help
my participation level.

If our commit-then-review policy is causing a large number of problems,
then we should examine the situation to see whether changing it is a
good tradeoff between quality and innovation.  I don't have a good sense
about whether it is the source of major issues.

===
Off on a tangent:

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.

https://issues.apache.org/jira/browse/ZOOKEEPER-2230?focusedCommentId=15751260=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-15751260

Solr tests are not in any kind of state where such an automatic QA
process could tell whether the patch actually made tests fail.  Erick is
trying to do something about that.

Thanks,
Shawn


-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Re: Code Reviews

[Dawid Weiss]

> I’d like to suggest that we start requiring code review to non-trivial 
> patches.

Don't know if it has to be a strict, corporate-like rule... Most folks
over here do get the gut feeling on what's non-trivial and requires a
second pair of eyes. JIRA and patch reviews have been serving this
purpose quite all right I think, although I recall a discussion of its
advantages and disadvantages (compared to github's review system, for
example). My concern is that making it a requirement isn't really
helping anyhow in attracting people to review those patches and is
creating a problem if you want to commit something larger, yet find
nobody interested in reviewing that patch.

Don't get me wrong, I know there are open source projects that do
require sign-offs and approvals; I'm just not sure we really need it
(or that it'd change anything in a substantial way).

Dawid

-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Re: Code Reviews

[David Smiley]

> To add to it, I think we should also wait before merging things to the
stable branch and commit only to master in case of non-trivial patches.

Maybe sometimes; a judgement call.  It can draw out how long it takes for
issues to get to completion; making it easier to forget that an issue isn't
quite complete yet.

On Wed, Feb 28, 2018 at 2:02 PM Anshum Gupta  wrote:

> +1 to the idea of code review before committing non-trivial patches.
>
> I do however worry about the cases when someone asks for feedback but
> doesn’t hear from anyone for reasonably long durations. In such situations
> perhaps a week should be good enough time to ask for feedback and wait
> before merging the code (to master).
>
> To add to it, I think we should also wait before merging things to the
> stable branch and commit only to master in case of non-trivial patches. I
> may be mixing two things here but I feel they are related. We used to
> almost always only commit to master and wait for stuff to bake until a
> while ago but I think that’s not the practice anymore.
>
> Overall, I’m +1 on this!
>
> Anshum
>
> On Feb 28, 2018, at 23:40, David Smiley  wrote:
>
> +1 I'm comfortable with that.   And I don't think this rule should apply
> to Solr alone; it should apply to Lucene as well, even though a greater
> percentage of issues there get reviews.
>
> I think we all appreciate the value of code reviews -- no convincing of
> that needed.  The challenge this will create is actually getting one,
> especially for those of us who submit patches that don't have
> collaborators.  This goes for a chunk of my work (Lucene/Solr alike).  I
> think I'll just ask/suggest for individuals to review that are likely to
> take an interest.
>
> On Wed, Feb 28, 2018 at 12:59 PM Tomas Fernandez Lobbe 
> wrote:
>
>> In an effort to improve code quality, I’d like to suggest that we start
>> requiring code review to non-trivial patches. Not sure if/how other open
>> source projects are doing code reviews, but I’ve been using it in internal
>> projects for many years and it’s a great way to catch bugs early, some of
>> them very difficult to catch in unit tests, like “You are breaking API
>> compatibility with this change”, or “you are swallowing
>> InterruptedExceptions”, etc. It is also a great way to standardize a bit
>> more our code base and to encourage community members to review and learn
>> then code.
>> In Lucene-land, this is already a common practice but on the Solr side is
>> rare to see. It is common on Solr that committer A doesn’t know much about
>> component X, so reviewing that may sound useless, but even in that case you
>> can provide feedback on the code itself being added (and in the meantime
>> learn something about component X).
>>
>> What do people think about it?
>>
>> Regarding tools to do it, I’m open to suggestions. I really like Github
>> PRs, that now are easy to integrate with Jira and you can create PRs from
>> forks or even from two existing branches of the official repo. Also, since
>> people is really familiar with them, I expect to encourage reviewers by
>> using them.
>>
>> Tomás
>>
> --
> Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker
> LinkedIn: http://linkedin.com/in/davidwsmiley | Book:
> http://www.solrenterprisesearchserver.com
>
> --
Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker
LinkedIn: http://linkedin.com/in/davidwsmiley | Book:
http://www.solrenterprisesearchserver.com

Re: Code Reviews

[Anshum Gupta]

+1 to the idea of code review before committing non-trivial patches. 

I do however worry about the cases when someone asks for feedback but doesn’t 
hear from anyone for reasonably long durations. In such situations perhaps a 
week should be good enough time to ask for feedback and wait before merging the 
code (to master). 

To add to it, I think we should also wait before merging things to the stable 
branch and commit only to master in case of non-trivial patches. I may be 
mixing two things here but I feel they are related. We used to almost always 
only commit to master and wait for stuff to bake until a while ago but I think 
that’s not the practice anymore.

Overall, I’m +1 on this!

Anshum

> On Feb 28, 2018, at 23:40, David Smiley  wrote:
> 
> +1 I'm comfortable with that.   And I don't think this rule should apply to 
> Solr alone; it should apply to Lucene as well, even though a greater 
> percentage of issues there get reviews.
> 
> I think we all appreciate the value of code reviews -- no convincing of that 
> needed.  The challenge this will create is actually getting one, especially 
> for those of us who submit patches that don't have collaborators.  This goes 
> for a chunk of my work (Lucene/Solr alike).  I think I'll just ask/suggest 
> for individuals to review that are likely to take an interest.
> 
>> On Wed, Feb 28, 2018 at 12:59 PM Tomas Fernandez Lobbe  
>> wrote:
>> In an effort to improve code quality, I’d like to suggest that we start 
>> requiring code review to non-trivial patches. Not sure if/how other open 
>> source projects are doing code reviews, but I’ve been using it in internal 
>> projects for many years and it’s a great way to catch bugs early, some of 
>> them very difficult to catch in unit tests, like “You are breaking API 
>> compatibility with this change”, or “you are swallowing 
>> InterruptedExceptions”, etc. It is also a great way to standardize a bit 
>> more our code base and to encourage community members to review and learn 
>> then code.
>> In Lucene-land, this is already a common practice but on the Solr side is 
>> rare to see. It is common on Solr that committer A doesn’t know much about 
>> component X, so reviewing that may sound useless, but even in that case you 
>> can provide feedback on the code itself being added (and in the meantime 
>> learn something about component X).
>> 
>> What do people think about it?
>> 
>> Regarding tools to do it, I’m open to suggestions. I really like Github PRs, 
>> that now are easy to integrate with Jira and you can create PRs from forks 
>> or even from two existing branches of the official repo. Also, since people 
>> is really familiar with them, I expect to encourage reviewers by using them.
>> 
>> Tomás
> -- 
> Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker
> LinkedIn: http://linkedin.com/in/davidwsmiley | Book: 
> http://www.solrenterprisesearchserver.com

Re: Code Reviews

[Joel Bernstein]

Ok, so it's clear what you're proposing then. You want to change the CTR
policy. That is indeed quite a big proposal. As I mentioned I'm personally
for CTR, but it would be good to hear other peoples thoughts on this.

Joel Bernstein
http://joelsolr.blogspot.com/

On Wed, Feb 28, 2018 at 1:30 PM, Tomas Fernandez Lobbe 
wrote:

> I’m not sure how CTR was put in place either, but it was done 10+ years
> ago, when Solr had less than 1/10 of the committers it has now and who
> knows how many less production deployments/users. Now Solr is a completely
> different project than back then, and what was the correct process then may
> not be the correct process now. I’m happy to trade some development speed
> for code quality.
>
> I think just saying “anyone can ask for a review” is not going to be good
> enough, this is the case right now, and it rarely happen.
>
> Tomás
>
>
> On Feb 28, 2018, at 10:17 AM, Joel Bernstein  wrote:
>
> I agree that code reviews would be a good idea. But to require code
> reviews before committing would be a big change in practice for the Solr
> committers. I'm not sure how the commit, then review policy was put in
> place or what it would mean to change that. Also I would probably
> personally vote against a change to the commit and the review policy.
>
> But, I would be open to encouraging a culture of code review like there is
> in Lucene.
>
> Joel Bernstein
> http://joelsolr.blogspot.com/
>
> On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe  > wrote:
>
>> In an effort to improve code quality, I’d like to suggest that we start
>> requiring code review to non-trivial patches. Not sure if/how other open
>> source projects are doing code reviews, but I’ve been using it in internal
>> projects for many years and it’s a great way to catch bugs early, some of
>> them very difficult to catch in unit tests, like “You are breaking API
>> compatibility with this change”, or “you are swallowing
>> InterruptedExceptions”, etc. It is also a great way to standardize a bit
>> more our code base and to encourage community members to review and learn
>> then code.
>> In Lucene-land, this is already a common practice but on the Solr side is
>> rare to see. It is common on Solr that committer A doesn’t know much about
>> component X, so reviewing that may sound useless, but even in that case you
>> can provide feedback on the code itself being added (and in the meantime
>> learn something about component X).
>>
>> What do people think about it?
>>
>> Regarding tools to do it, I’m open to suggestions. I really like Github
>> PRs, that now are easy to integrate with Jira and you can create PRs from
>> forks or even from two existing branches of the official repo. Also, since
>> people is really familiar with them, I expect to encourage reviewers by
>> using them.
>>
>> Tomás
>>
>
>
>

Re: Code Reviews

[Tomas Fernandez Lobbe]

I’m not sure how CTR was put in place either, but it was done 10+ years ago, 
when Solr had less than 1/10 of the committers it has now and who knows how 
many less production deployments/users. Now Solr is a completely different 
project than back then, and what was the correct process then may not be the 
correct process now. I’m happy to trade some development speed for code quality.

I think just saying “anyone can ask for a review” is not going to be good 
enough, this is the case right now, and it rarely happen. 

Tomás

> On Feb 28, 2018, at 10:17 AM, Joel Bernstein  wrote:
> 
> I agree that code reviews would be a good idea. But to require code reviews 
> before committing would be a big change in practice for the Solr committers. 
> I'm not sure how the commit, then review policy was put in place or what it 
> would mean to change that. Also I would probably personally vote against a 
> change to the commit and the review policy.
> 
> But, I would be open to encouraging a culture of code review like there is in 
> Lucene.
> 
> Joel Bernstein
> http://joelsolr.blogspot.com/ 
> 
> On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe  > wrote:
> In an effort to improve code quality, I’d like to suggest that we start 
> requiring code review to non-trivial patches. Not sure if/how other open 
> source projects are doing code reviews, but I’ve been using it in internal 
> projects for many years and it’s a great way to catch bugs early, some of 
> them very difficult to catch in unit tests, like “You are breaking API 
> compatibility with this change”, or “you are swallowing 
> InterruptedExceptions”, etc. It is also a great way to standardize a bit more 
> our code base and to encourage community members to review and learn then 
> code.
> In Lucene-land, this is already a common practice but on the Solr side is 
> rare to see. It is common on Solr that committer A doesn’t know much about 
> component X, so reviewing that may sound useless, but even in that case you 
> can provide feedback on the code itself being added (and in the meantime 
> learn something about component X).
> 
> What do people think about it?
> 
> Regarding tools to do it, I’m open to suggestions. I really like Github PRs, 
> that now are easy to integrate with Jira and you can create PRs from forks or 
> even from two existing branches of the official repo. Also, since people is 
> really familiar with them, I expect to encourage reviewers by using them.
> 
> Tomás
> 

Re: Code Reviews

[Joel Bernstein]

I agree that code reviews would be a good idea. But to require code reviews
before committing would be a big change in practice for the Solr
committers. I'm not sure how the commit, then review policy was put in
place or what it would mean to change that. Also I would probably
personally vote against a change to the commit and the review policy.

But, I would be open to encouraging a culture of code review like there is
in Lucene.

Joel Bernstein
http://joelsolr.blogspot.com/

On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe 
wrote:

> In an effort to improve code quality, I’d like to suggest that we start
> requiring code review to non-trivial patches. Not sure if/how other open
> source projects are doing code reviews, but I’ve been using it in internal
> projects for many years and it’s a great way to catch bugs early, some of
> them very difficult to catch in unit tests, like “You are breaking API
> compatibility with this change”, or “you are swallowing
> InterruptedExceptions”, etc. It is also a great way to standardize a bit
> more our code base and to encourage community members to review and learn
> then code.
> In Lucene-land, this is already a common practice but on the Solr side is
> rare to see. It is common on Solr that committer A doesn’t know much about
> component X, so reviewing that may sound useless, but even in that case you
> can provide feedback on the code itself being added (and in the meantime
> learn something about component X).
>
> What do people think about it?
>
> Regarding tools to do it, I’m open to suggestions. I really like Github
> PRs, that now are easy to integrate with Jira and you can create PRs from
> forks or even from two existing branches of the official repo. Also, since
> people is really familiar with them, I expect to encourage reviewers by
> using them.
>
> Tomás
>

Re: Code Reviews

[David Smiley]

+1 I'm comfortable with that.   And I don't think this rule should apply to
Solr alone; it should apply to Lucene as well, even though a greater
percentage of issues there get reviews.

I think we all appreciate the value of code reviews -- no convincing of
that needed.  The challenge this will create is actually getting one,
especially for those of us who submit patches that don't have
collaborators.  This goes for a chunk of my work (Lucene/Solr alike).  I
think I'll just ask/suggest for individuals to review that are likely to
take an interest.

On Wed, Feb 28, 2018 at 12:59 PM Tomas Fernandez Lobbe 
wrote:

> In an effort to improve code quality, I’d like to suggest that we start
> requiring code review to non-trivial patches. Not sure if/how other open
> source projects are doing code reviews, but I’ve been using it in internal
> projects for many years and it’s a great way to catch bugs early, some of
> them very difficult to catch in unit tests, like “You are breaking API
> compatibility with this change”, or “you are swallowing
> InterruptedExceptions”, etc. It is also a great way to standardize a bit
> more our code base and to encourage community members to review and learn
> then code.
> In Lucene-land, this is already a common practice but on the Solr side is
> rare to see. It is common on Solr that committer A doesn’t know much about
> component X, so reviewing that may sound useless, but even in that case you
> can provide feedback on the code itself being added (and in the meantime
> learn something about component X).
>
> What do people think about it?
>
> Regarding tools to do it, I’m open to suggestions. I really like Github
> PRs, that now are easy to integrate with Jira and you can create PRs from
> forks or even from two existing branches of the official repo. Also, since
> people is really familiar with them, I expect to encourage reviewers by
> using them.
>
> Tomás
>
-- 
Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker
LinkedIn: http://linkedin.com/in/davidwsmiley | Book:
http://www.solrenterprisesearchserver.com