Re: PR process and etiquette

[Darrel Schneider]

+1 for adding a CONTRIBUTING.MD file

From: Sarah Abbey 
Sent: Thursday, October 29, 2020 2:07 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

Regarding knowing who to tag in a PR, because I am working on a very specific 
aspect of Geode, it was frustrating before I was a committer to not be able to 
add reviewers since I knew the handful of people that had enough context to 
review most of the PRs I submitted.  But it is also not hard for me to ask 
these people directly since I work with them every day.  I can imagine it would 
be very frustrating to know exactly who to request a review from, but not be 
able to add them and not be able to directly contact them or get their 
attention.  It would also be nice to be able to request reviews from people who 
are not committers, which we may not be able to change due to GitHub 
limitations. Some of my team members are not committers yet, but I still value 
their input/review of the code even if their review would not count as an 
official approving review. Or if we submit a PR that solves an issue raised by 
a non-committer, it would be nice to have them review it (if they want to/it 
makes sense for them to) to be sure the issue is addressed. Robert has 
mentioned that a workaround is to tag those users in a comment.

Since I only feel like I have significant context in one area of Geode, I 
usually scan the list of PRs for that area unless I'm explicitly tagged as a 
reviewer.  Sometimes, I read other PRs to gain knowledge or if I'm curious, but 
I don't usually add a review.

Regarding waiting until all commit checks are green, it depends on the PR for 
me.  I usually glance at a PR once I'm tagged as a reviewer or I see something 
that needs a review in my area of expertise.  If it looks like the PR still 
needs a lot of work and checks like build did not pass, then I typically wait 
to review it.  If most of the checks have passed, and checks that take a long 
time to run, like DUnit or Acceptance, haven't completed, I will often review 
it.  If certain checks that sometimes have flaky tests are not passing, like 
DUnit, and all other checks are green, I'll often look at those failures to see 
if they are related to the PR at all and check Jira to see if there has been an 
issue filed for them or not.  I'll still review the PR and make a comment about 
the flaky test.  If the failure seems related to the PR, I might still review 
it to see what might've caused the failure.

Whatever we do, our guide to 
contributing
 could definitely use an update.  We might even consider putting a 
CONTRIBUTING.MD file directly in our GitHub repo.  I've found these guides 
useful when contributing to other open source projects.  I also like 
contributing to open source projects when my PR is reviewed timely (within a 
week, though I'm sure we all have different definitions of timely) and any 
feedback or discussion is constructive and kind.

Sarah

From: Donal Evans 
Sent: Thursday, October 29, 2020 3:41 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

As a (relatively) new committer, one of the more difficult aspects of the PR 
process is knowing who to tag as a reviewer. The last person to touch a class 
may not actually have the context or depth of knowledge needed for a thorough 
review if, say, their changes were just refactoring or code cleanup, and if you 
don't have the luxury of working directly with other committers, it's not 
always clear who is the "right" person to ask for review. Add to that the fear 
of overburdening the more knowledgeable committers with endless requests for 
review, and you can find yourself in a position where the reviews you do end up 
getting are somewhat perfunctory or only address surface-level issues rather 
than potential more serious and fundamental problems.

The reverse of this is also something I have struggled with as one of the newer 
and less knowledgeable members of the community, as I'll sometimes see a PR sat 
waiting for review that changes areas of the code that I don't know much about 
and, wanting to help out, make some comments or requests for changes related to 
things like test naming, code style or other mostly cosmetic issues. Once those 
have been addressed, I can approve the PR, but I know that I haven't really​ 
reviewed it to the standard necessary to have confidence that it's not going to 
break something. On the one hand, I want to be active and help ensure the 
quality of code 

Re: PR process and etiquette

[Sarah Abbey]

Regarding knowing who to tag in a PR, because I am working on a very specific 
aspect of Geode, it was frustrating before I was a committer to not be able to 
add reviewers since I knew the handful of people that had enough context to 
review most of the PRs I submitted.  But it is also not hard for me to ask 
these people directly since I work with them every day.  I can imagine it would 
be very frustrating to know exactly who to request a review from, but not be 
able to add them and not be able to directly contact them or get their 
attention.  It would also be nice to be able to request reviews from people who 
are not committers, which we may not be able to change due to GitHub 
limitations. Some of my team members are not committers yet, but I still value 
their input/review of the code even if their review would not count as an 
official approving review. Or if we submit a PR that solves an issue raised by 
a non-committer, it would be nice to have them review it (if they want to/it 
makes sense for them to) to be sure the issue is addressed. Robert has 
mentioned that a workaround is to tag those users in a comment.

Since I only feel like I have significant context in one area of Geode, I 
usually scan the list of PRs for that area unless I'm explicitly tagged as a 
reviewer.  Sometimes, I read other PRs to gain knowledge or if I'm curious, but 
I don't usually add a review.

Regarding waiting until all commit checks are green, it depends on the PR for 
me.  I usually glance at a PR once I'm tagged as a reviewer or I see something 
that needs a review in my area of expertise.  If it looks like the PR still 
needs a lot of work and checks like build did not pass, then I typically wait 
to review it.  If most of the checks have passed, and checks that take a long 
time to run, like DUnit or Acceptance, haven't completed, I will often review 
it.  If certain checks that sometimes have flaky tests are not passing, like 
DUnit, and all other checks are green, I'll often look at those failures to see 
if they are related to the PR at all and check Jira to see if there has been an 
issue filed for them or not.  I'll still review the PR and make a comment about 
the flaky test.  If the failure seems related to the PR, I might still review 
it to see what might've caused the failure.

Whatever we do, our guide to 
contributing
 could definitely use an update.  We might even consider putting a 
CONTRIBUTING.MD file directly in our GitHub repo.  I've found these guides 
useful when contributing to other open source projects.  I also like 
contributing to open source projects when my PR is reviewed timely (within a 
week, though I'm sure we all have different definitions of timely) and any 
feedback or discussion is constructive and kind.

Sarah

From: Donal Evans 
Sent: Thursday, October 29, 2020 3:41 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

As a (relatively) new committer, one of the more difficult aspects of the PR 
process is knowing who to tag as a reviewer. The last person to touch a class 
may not actually have the context or depth of knowledge needed for a thorough 
review if, say, their changes were just refactoring or code cleanup, and if you 
don't have the luxury of working directly with other committers, it's not 
always clear who is the "right" person to ask for review. Add to that the fear 
of overburdening the more knowledgeable committers with endless requests for 
review, and you can find yourself in a position where the reviews you do end up 
getting are somewhat perfunctory or only address surface-level issues rather 
than potential more serious and fundamental problems.

The reverse of this is also something I have struggled with as one of the newer 
and less knowledgeable members of the community, as I'll sometimes see a PR sat 
waiting for review that changes areas of the code that I don't know much about 
and, wanting to help out, make some comments or requests for changes related to 
things like test naming, code style or other mostly cosmetic issues. Once those 
have been addressed, I can approve the PR, but I know that I haven't really​ 
reviewed it to the standard necessary to have confidence that it's not going to 
break something. On the one hand, I want to be active and help ensure the 
quality of code contributions in any way that I can, but on the other, I don't 
want my approval of a set of changes based on my limited understanding to be 
taken as a solid confirmation that there are no problems with them.

In terms of things that make the PR process easier as a reviewer, marking PRs 
as drafts until they're ready for comprehensive review is good, but I also have 
no problem with offering comments on a draft PR if they relate to things that 
are unlikely to change, like method or variable names, or even the broad 
approach being taken, so I don't 

Re: PR process and etiquette

[Donal Evans]

As a (relatively) new committer, one of the more difficult aspects of the PR 
process is knowing who to tag as a reviewer. The last person to touch a class 
may not actually have the context or depth of knowledge needed for a thorough 
review if, say, their changes were just refactoring or code cleanup, and if you 
don't have the luxury of working directly with other committers, it's not 
always clear who is the "right" person to ask for review. Add to that the fear 
of overburdening the more knowledgeable committers with endless requests for 
review, and you can find yourself in a position where the reviews you do end up 
getting are somewhat perfunctory or only address surface-level issues rather 
than potential more serious and fundamental problems.

The reverse of this is also something I have struggled with as one of the newer 
and less knowledgeable members of the community, as I'll sometimes see a PR sat 
waiting for review that changes areas of the code that I don't know much about 
and, wanting to help out, make some comments or requests for changes related to 
things like test naming, code style or other mostly cosmetic issues. Once those 
have been addressed, I can approve the PR, but I know that I haven't really​ 
reviewed it to the standard necessary to have confidence that it's not going to 
break something. On the one hand, I want to be active and help ensure the 
quality of code contributions in any way that I can, but on the other, I don't 
want my approval of a set of changes based on my limited understanding to be 
taken as a solid confirmation that there are no problems with them.

In terms of things that make the PR process easier as a reviewer, marking PRs 
as drafts until they're ready for comprehensive review is good, but I also have 
no problem with offering comments on a draft PR if they relate to things that 
are unlikely to change, like method or variable names, or even the broad 
approach being taken, so I don't view the draft status as a barrier to review. 
I also find it very helpful when the PR description gives an overview of what 
the PR is doing, not just repeating what's in the JIRA ticket associated with 
it, since the description of a bug and the description of its fix are often 
very different. Along a similar vein, descriptive commit messages are valuable 
since they can help provide context or motivation for why changes were made. 
The more I understand the contents of a PR in terms of what is being changed, 
why, and what the (intended) consequences are, the more confident I feel in 
being able to provide a thorough review.

Donal

From: Udo Kohlmeyer 
Sent: Wednesday, October 28, 2020 5:50 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

So far I would like to thank everyone for their thoughts and input.

@Dave, I would love to find a solution to the partial sign-off. I’ve been 
experimenting with the “Projects” setting. I wonder if we cannot have a 
“Documentation Check” project, that is added to every PR as a default project. 
We could have different states with the project, which would allow the docs 
folk to know what PRs are new and which still need to be reviewed for docs 
changes.

Now, I don’t know if we can restrict the merging of a PR based upon a state in 
the Project, but at the very least it will provide the ability to have an 
overview of PR with/without docs review. You can have a look at the “Quality 
Review” project I have created. Which I use to track all PRs that I would like 
to review for quality purposes. (code, structure, tests, etc)… I think Docs 
could have something similar.

@Bruce, I’m not trying to create another rule for the sake of creating a rule. 
Why do you believe that we as a community will give any submitter a stink-eye 
just because they did not submit a draft? I certainly would not. I would 
suggest that the submitter maybe submit a draft IF the PR is not in a ready 
state and needs a few more iterations to get to a ready state.

I believe it is easier and better for committers to go through a list of PRs to 
review if they know that the PR passes all of the testing checks.. As a failure 
in one area might actually cause some code components to change. Which might 
void an earlier review of the code. Also, I’m not suggesting that there are no 
reviews before the commit checks go green. You can easily request someone else 
to review whilst in a draft state.

As for knowing what reviewers to tag for a review is more limiting. How would I 
as a new PR submitter know WHO I should tag in the PR? Over time we have built 
up a great understanding of who might be a good person to review our code. But 
for a new community member, they do not know this. For them, they submit the 
PR, and someone in the community will review it.

I would also like everyone to think back on their own approach on deciding what 
PRs to review.

Do you look at the PR and decide to wait until all commit 

Re: PR process and etiquette

[Alberto Gomez]

Hi there,

Here come my 2 cents.

@Udo Kohlmeyer, thanks for your proposals to make this 
community better, and also for your willingness to get feedback from people who 
are new to the community.

In my experience, one of the tricky parts working in the community is getting 
reviewers for PRs. It is a bit of a mystery what will happen once you submit a 
PR. Sometimes you get a review in a few hours. Sometimes you do not and ask in 
the list for reviewers and after that, sometimes you get reviewers soon, and 
sometimes you don't, and you need to insist in the list. I have sometimes asked 
for a review to a particular person via e-mail as I do not have permissions to 
assign reviewers to PRs and sometimes have not received any answer.
I figure the response time is very dependent on how busy people. Anyhow, it is 
the uncertainty of what is going on behind the scenes what makes things hard.
If any proposal makes the review process more predictable, I am up for it. I 
think Udo's reflections to come up with a consistent approach to the review 
process are very valuable.

In my opinion, it is a good idea to submit draft PRs while we do not have the 
green light from the CI. I have many times submitted PRs, gone to sleep just to 
realize the morning after that some test cases in the CI failed (either due to 
flaky test cases or due to my changes). Sometimes I had already gotten a review 
and I would have preferred to have it once the CI was clear. Other times I did 
not get a review and I wondered if that (or those) failures would keep 
reviewers away from my PR given that they once looked at it and had test cases 
failures.

Alberto G.

From: Udo Kohlmeyer 
Sent: Thursday, October 29, 2020 1:50 AM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

So far I would like to thank everyone for their thoughts and input.

@Dave, I would love to find a solution to the partial sign-off. I’ve been 
experimenting with the “Projects” setting. I wonder if we cannot have a 
“Documentation Check” project, that is added to every PR as a default project. 
We could have different states with the project, which would allow the docs 
folk to know what PRs are new and which still need to be reviewed for docs 
changes.

Now, I don’t know if we can restrict the merging of a PR based upon a state in 
the Project, but at the very least it will provide the ability to have an 
overview of PR with/without docs review. You can have a look at the “Quality 
Review” project I have created. Which I use to track all PRs that I would like 
to review for quality purposes. (code, structure, tests, etc)… I think Docs 
could have something similar.

@Bruce, I’m not trying to create another rule for the sake of creating a rule. 
Why do you believe that we as a community will give any submitter a stink-eye 
just because they did not submit a draft? I certainly would not. I would 
suggest that the submitter maybe submit a draft IF the PR is not in a ready 
state and needs a few more iterations to get to a ready state.

I believe it is easier and better for committers to go through a list of PRs to 
review if they know that the PR passes all of the testing checks.. As a failure 
in one area might actually cause some code components to change. Which might 
void an earlier review of the code. Also, I’m not suggesting that there are no 
reviews before the commit checks go green. You can easily request someone else 
to review whilst in a draft state.

As for knowing what reviewers to tag for a review is more limiting. How would I 
as a new PR submitter know WHO I should tag in the PR? Over time we have built 
up a great understanding of who might be a good person to review our code. But 
for a new community member, they do not know this. For them, they submit the 
PR, and someone in the community will review it.

I would also like everyone to think back on their own approach on deciding what 
PRs to review.

Do you look at the PR and decide to wait until all commit checks are green?
Do you go through the list and find one, that you think you can review, whilst 
the commit checks are still running?
Do you only review PRs in which you have been explicitly tagged?
Do you scan the PRs for a commit in an area of “expertise”?
Do you scan the PRs for committers that you know?

Whatever approach we take, I would like us to come up with an approach, that we 
as a community follow, to have a consistent approach to the review.
A consistent way we can evaluate if the code is in a “ready” state?
A consistent way, that the community will know, that when they submit the PR it 
will be looked at.
A consistent way that I, as a committer, will know that if I spend the time to 
review the PR will not be a waste of my time, because it wasn’t ready.

I don’t think community members are repulsed by a project with structure, but I 
do know that I question a project without structure and one where it takes a 

Re: PR process and etiquette

[Bruce Schuchardt]

I totally agree that folks should use Draft mode for things that aren't ready 
for review.  Beyond that it's easy enough to look at a PR and see if it's in 
Draft state or hasn't passed checks before putting in an effort to review the 
code.  However, going into the code of a Draft PR or one that hasn't passed the 
checks can be useful in that you can give early feedback if you see problems.

For the partial-signoff problem I've always just left a comment that the 
portion of the PR that I feel competent to review is ready to merge.  I don't 
give the PR an approval in that case.

On 10/28/20, 5:50 PM, "Udo Kohlmeyer"  wrote:

So far I would like to thank everyone for their thoughts and input.

@Dave, I would love to find a solution to the partial sign-off. I’ve been 
experimenting with the “Projects” setting. I wonder if we cannot have a 
“Documentation Check” project, that is added to every PR as a default project. 
We could have different states with the project, which would allow the docs 
folk to know what PRs are new and which still need to be reviewed for docs 
changes.

Now, I don’t know if we can restrict the merging of a PR based upon a state 
in the Project, but at the very least it will provide the ability to have an 
overview of PR with/without docs review. You can have a look at the “Quality 
Review” project I have created. Which I use to track all PRs that I would like 
to review for quality purposes. (code, structure, tests, etc)… I think Docs 
could have something similar.

@Bruce, I’m not trying to create another rule for the sake of creating a 
rule. Why do you believe that we as a community will give any submitter a 
stink-eye just because they did not submit a draft? I certainly would not. I 
would suggest that the submitter maybe submit a draft IF the PR is not in a 
ready state and needs a few more iterations to get to a ready state.

I believe it is easier and better for committers to go through a list of 
PRs to review if they know that the PR passes all of the testing checks.. As a 
failure in one area might actually cause some code components to change. Which 
might void an earlier review of the code. Also, I’m not suggesting that there 
are no reviews before the commit checks go green. You can easily request 
someone else to review whilst in a draft state.

As for knowing what reviewers to tag for a review is more limiting. How 
would I as a new PR submitter know WHO I should tag in the PR? Over time we 
have built up a great understanding of who might be a good person to review our 
code. But for a new community member, they do not know this. For them, they 
submit the PR, and someone in the community will review it.

I would also like everyone to think back on their own approach on deciding 
what PRs to review.

Do you look at the PR and decide to wait until all commit checks are green?
Do you go through the list and find one, that you think you can review, 
whilst the commit checks are still running?
Do you only review PRs in which you have been explicitly tagged?
Do you scan the PRs for a commit in an area of “expertise”?
Do you scan the PRs for committers that you know?

Whatever approach we take, I would like us to come up with an approach, 
that we as a community follow, to have a consistent approach to the review.
A consistent way we can evaluate if the code is in a “ready” state?
A consistent way, that the community will know, that when they submit the 
PR it will be looked at.
A consistent way that I, as a committer, will know that if I spend the time 
to review the PR will not be a waste of my time, because it wasn’t ready.

I don’t think community members are repulsed by a project with structure, 
but I do know that I question a project without structure and one where it 
takes a long time to have a PR reviewed.

I would also wlecome our newer community members (anyone who has been a 
committer for less than 18months or anyone who is not a committer) to comment 
here. Is there anything in particular that attracts or repulses you when it 
comes to contributing to the project.

--Udo

From: Dave Barnes 
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette
Here's a common use case that we should address: A single PR may require
two reviews, one for code and another for docs, before it can be said to be
fully reviewed and ready to merge.
Points to consider:

   - Many PRs, especially those introducing new features or user-settable
   properties, include both code and docs. "Docs" includes updates to the 
user
   guide sources, but also code comments that are consumed by community
   developers and are included in the auto-generated API documentation.
   Parameter name choices, explanations, spelling and grammar need to be 
right.
   - Reviews of code and docs require different skills,