Code Reviews

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Code Reviews

Tomas Fernandez Lobbe-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

david.w.smiley@gmail.com
+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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Joel Bernstein
In reply to this post by Tomas Fernandez Lobbe-2
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.


On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Tomas Fernandez Lobbe-2
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 <[hidden email]> 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.


On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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.


On Wed, Feb 28, 2018 at 1:30 PM, Tomas Fernandez Lobbe <[hidden email]> 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 <[hidden email]> 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.


On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Anshum Gupta-3
In reply to this post by david.w.smiley@gmail.com
+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 <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

david.w.smiley@gmail.com
> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
--
Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker
Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Dawid Weiss-2
In reply to this post by Tomas Fernandez Lobbe-2
> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Shawn Heisey-2
In reply to this post by Tomas Fernandez Lobbe-2
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&page=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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Cassandra Targett

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

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Tomas Fernandez Lobbe-2

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


Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Stefan Matheis-3
> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> a écrit :
On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey <[hidden email]> 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. 



Reply | Threaded
Open this post in threaded view
|

Re: Code Reviews

Varun Thacker-4
+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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> a écrit :
On Wed, Feb 28, 2018 at 1:58 PM, Shawn Heisey <[hidden email]> 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.