Re: PR process and etiquette

[Udo Kohlmeyer]

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, so a good-quality
   PR review in such cases benefits from multiple reviewers, (at least) one
   for code, and (at least) one for docs.
   - [My key concern] Current protocol is 1 reviewer approval qualifies the
   PR for merging. I flag my doc reviews in such cases with a note to the
   submitter saying something like "Docs reviewed and approved; technical
   review still needed before merging". But a submitter who's eager to merge
   may just see green and go for it, without reading the caveat. I'm against
   enforcing a multiple-review requirement. What I would like to see is a
   status for partial approval, so I can register my approval without
   prematurely opening the door to a code merge.
   - Other considerations:
  - There are PRs that don't fit 

Re: PR process and etiquette

[Dave Barnes]

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, so a good-quality
   PR review in such cases benefits from multiple reviewers, (at least) one
   for code, and (at least) one for docs.
   - [My key concern] Current protocol is 1 reviewer approval qualifies the
   PR for merging. I flag my doc reviews in such cases with a note to the
   submitter saying something like "Docs reviewed and approved; technical
   review still needed before merging". But a submitter who's eager to merge
   may just see green and go for it, without reading the caveat. I'm against
   enforcing a multiple-review requirement. What I would like to see is a
   status for partial approval, so I can register my approval without
   prematurely opening the door to a code merge.
   - Other considerations:
  - There are PRs that don't fit this two-part use case. Many PRs are
  code-only, with no doc component. Conversely, some PRs are
docs-only, with
  no code component.
  - PRs often are not recognized as falling into this two-part use
  case. Submitters of JIRA tickets may not know whether the solution to the
  problem they're reporting will affect docs. Submitter of the PR
addressing
  the JIRA ticket may not think to add the 'docs' component to the related
  JIRA ticket or to request a doc reviewer when creating the PR.


On Wed, Oct 28, 2020 at 8:41 AM Robert Houghton 
wrote:

> There are some pieces of Apache infrastructure we can control without
> needing tickets: Specifically
> required_pull_request_reviews:
> dismiss_stale_reviews: true
> require_code_owner_reviews: true
>
> I think these specific controls could go a long way towards helping keep
> our PR quality high, while reducing cognitive load for the reviewers.
>
> Full documentation here<
> https://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features#git.asf.yamlfeatures-BranchProtection
> >
>
>
>
> From: Bruce Schuchardt 
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org 
> Subject: Re: PR process and etiquette
> -1
> While I often use the Draft option I don't see why we want to add even
> more rules about how we use github.  I think it's enough to put in a PR and
> then add reviewers when you're ready for comments.  Getting the stink-eye
> for putting up a non-Draft PR is just going to make it more difficult to
> attract and retain new contributors.
>
> On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:
>
> Dear Apache Geode Devs,
> It is really great going through all the PRs that been submitted. As
> Josh Long is known to say: "I work for PRs".
> Whilst going through some of the PRs I do see that there are many PRs
> that have multiple commits against the PR.
> I know that the PR submission framework kicks off more testing than we
> do on our own local machines. It is extremely uncommon to submit a PR the
> first time and have all tests go green. Which h means we invariably iterate
> over commits to make the build go green.
> In this limbo time period, it is hard for any reviewer to know when
> the ticket is ready to be reviewed.
> I want to propose that when submitting a PR, it is initially submitted
> as a DRAFT PR. This way, all test can still be run and work can be done to
> make sure "green" is achieved. Once "green" status has been achieved, the
> draft can then be upgraded to a final PR by pressing the "Ready For Review"
> button. At this point all commits on the branch can then once again be
> squashed into a single commit.
> Now project committers will now know that the PR is in a state that it
> can be reviewed for completeness and functionality.
> In addition, it will help tremendously helpful if anyone submitting a
> PR monitors their PR for activity. If there is no activity for a few days,
> please feel free to ping the Apache Geode Dev community for review. If
> review is request, please prioritize some time to address the feedback, as
> reviewers spend time reviewing code and getting an understanding what the
> code is doing. If too much time goes by, between review and addressing the
> review comments, not only does the reviewer lose context, possibly
> requiring them to spend time again to understand what the code was/is
> supposed to do, but also possibly lose interest, as the ticket has now
> become cold or dropped down the list of PRs.
> There are currently 

Re: PR process and etiquette

[Kirk Lund]

How about we make this a recommendation rather than a rule?

I'd like to also recommend that contributors consider prefixing the PR
title with "DRAFT: " while it is in draft. This just makes it easier to see
at a glance that it's a draft. When I change the PR to "ready for review" I
edit the title to remove "DRAFT: ".

On Wed, Oct 28, 2020 at 9:59 AM Bruce Schuchardt  wrote:

> Hi Owen - I wasn't aware that non-committers can't add reviewers to their
> PRs but I don't see how using DRAFT mode helps with that.  The idea that I
> can't request a review until the commit checks all pass seems absurd to me.
>
> On 10/28/20, 9:15 AM, "Owen Nichols"  wrote:
>
> Hey Bruce, please consider that non-committers are not permitted to
> add reviewers themselves, so a consistent convention to indicate when a PR
> has moved from work-in-progress to ready-for-review will help alert the
> community when to assign reviewers.
>
> Currently, I see countless creative variations on people adding "WIP"
> or "DO NOT REVIEW" or other text somewhere in the summary or description.
> I think the suggestion here is to standardize on using Draft status as the
> canonical way to communicate this.
>
> Also worth noting, you can change a PR back to 'draft' mode at any
> time (so whether you forgot initially, or a test failure or review feedback
> made you realize more work is needed, you can always amend the draft status
> to communicate whether you're still working, or ready for review again).
>
> I'm not sure why you think this is going to make it more difficult to
> attract and retain new contributors.  The point is to make it easier for
> new contributors to get their PRs reviewed, and give reviewers more
> confident that their time is appreciated.
>
> I see your point that as a committer, you have no need to communicate
> this status, since you can simply add reviewers yourself when you're
> ready.  Asking everyone to use draft status is not the ideal workaround for
> the GitHub flaw that non-committers can't request reviewers, but it also
> takes only 2 seconds of your time.  Expecting new contributors to email the
> dev list every time they want to request a review of their PR seems far
> more problematic in term of attracting and retaining new contributors.
>
> On 10/28/20, 8:10 AM, "Bruce Schuchardt"  wrote:
>
> -1
> While I often use the Draft option I don't see why we want to add
> even more rules about how we use github.  I think it's enough to put in a
> PR and then add reviewers when you're ready for comments.  Getting the
> stink-eye for putting up a non-Draft PR is just going to make it more
> difficult to attract and retain new contributors.
>
> On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:
>
> Dear Apache Geode Devs,
> It is really great going through all the PRs that been
> submitted. As Josh Long is known to say: "I work for PRs".
> Whilst going through some of the PRs I do see that there are
> many PRs that have multiple commits against the PR.
> I know that the PR submission framework kicks off more testing
> than we do on our own local machines. It is extremely uncommon to submit a
> PR the first time and have all tests go green. Which h means we invariably
> iterate over commits to make the build go green.
> In this limbo time period, it is hard for any reviewer to know
> when the ticket is ready to be reviewed.
> I want to propose that when submitting a PR, it is initially
> submitted as a DRAFT PR. This way, all test can still be run and work can
> be done to make sure "green" is achieved. Once "green" status has been
> achieved, the draft can then be upgraded to a final PR by pressing the
> "Ready For Review" button. At this point all commits on the branch can then
> once again be squashed into a single commit.
> Now project committers will now know that the PR is in a state
> that it can be reviewed for completeness and functionality.
> In addition, it will help tremendously helpful if anyone
> submitting a PR monitors their PR for activity. If there is no activity for
> a few days, please feel free to ping the Apache Geode Dev community for
> review. If review is request, please prioritize some time to address the
> feedback, as reviewers spend time reviewing code and getting an
> understanding what the code is doing. If too much time goes by, between
> review and addressing the review comments, not only does the reviewer lose
> context, possibly requiring them to spend time again to understand what the
> code was/is supposed to do, but also possibly lose interest, as the ticket
> has now become cold or dropped down the list of PRs.
> There are currently many PRs that are in a cold state, where
> the time between review and response has been so long, that both parties
> (reviewer and submitter) have forgotten about the PR.
> In the case that 

Full diffs in Jira comments

[Kirk Lund]

Anyone else notice that we're getting full diffs as comments to Jira
tickets again?

Re: PR process and etiquette

[Bruce Schuchardt]

Hi Owen - I wasn't aware that non-committers can't add reviewers to their PRs 
but I don't see how using DRAFT mode helps with that.  The idea that I can't 
request a review until the commit checks all pass seems absurd to me.

On 10/28/20, 9:15 AM, "Owen Nichols"  wrote:

Hey Bruce, please consider that non-committers are not permitted to add 
reviewers themselves, so a consistent convention to indicate when a PR has 
moved from work-in-progress to ready-for-review will help alert the community 
when to assign reviewers.

Currently, I see countless creative variations on people adding "WIP" or 
"DO NOT REVIEW" or other text somewhere in the summary or description.  I think 
the suggestion here is to standardize on using Draft status as the canonical 
way to communicate this.  

Also worth noting, you can change a PR back to 'draft' mode at any time (so 
whether you forgot initially, or a test failure or review feedback made you 
realize more work is needed, you can always amend the draft status to 
communicate whether you're still working, or ready for review again).

I'm not sure why you think this is going to make it more difficult to 
attract and retain new contributors.  The point is to make it easier for new 
contributors to get their PRs reviewed, and give reviewers more confident that 
their time is appreciated.

I see your point that as a committer, you have no need to communicate this 
status, since you can simply add reviewers yourself when you're ready.  Asking 
everyone to use draft status is not the ideal workaround for the GitHub flaw 
that non-committers can't request reviewers, but it also takes only 2 seconds 
of your time.  Expecting new contributors to email the dev list every time they 
want to request a review of their PR seems far more problematic in term of 
attracting and retaining new contributors.

On 10/28/20, 8:10 AM, "Bruce Schuchardt"  wrote:

-1
While I often use the Draft option I don't see why we want to add even 
more rules about how we use github.  I think it's enough to put in a PR and 
then add reviewers when you're ready for comments.  Getting the stink-eye for 
putting up a non-Draft PR is just going to make it more difficult to attract 
and retain new contributors.

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. 
As Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many 
PRs that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than 
we do on our own local machines. It is extremely uncommon to submit a PR the 
first time and have all tests go green. Which h means we invariably iterate 
over commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when 
the ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially 
submitted as a DRAFT PR. This way, all test can still be run and work can be 
done to make sure "green" is achieved. Once "green" status has been achieved, 
the draft can then be upgraded to a final PR by pressing the "Ready For Review" 
button. At this point all commits on the branch can then once again be squashed 
into a single commit.
Now project committers will now know that the PR is in a state that 
it can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting 
a PR monitors their PR for activity. If there is no activity for a few days, 
please feel free to ping the Apache Geode Dev community for review. If review 
is request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the 
time between review and response has been so long, that both parties (reviewer 
and submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than 
expected, please downgrade the PR to draft status again. At this point, it does 
not mean that reviewers will not be able to help anymore, as you can request a 
reviewer to help with feedback and comments, until one feels that the PR is 
back in a state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community 

Re: PR process and etiquette

[Owen Nichols]

For the narrow goal of making it easier for non-committers to get reviewers for 
their PRs, we could also consider defining a "reviewers wanted" label.  However 
it might not be very obvious to new committers that they need to click the gear 
to look for that label, unless we also update the PR template text to mention 
it.

Using lack-of-draft-status as the ready-for-review indicator does have the 
advantage of being the 'default' state.


On 10/28/20, 9:15 AM, "Owen Nichols"  wrote:

Hey Bruce, please consider that non-committers are not permitted to add 
reviewers themselves, so a consistent convention to indicate when a PR has 
moved from work-in-progress to ready-for-review will help alert the community 
when to assign reviewers.

Currently, I see countless creative variations on people adding "WIP" or 
"DO NOT REVIEW" or other text somewhere in the summary or description.  I think 
the suggestion here is to standardize on using Draft status as the canonical 
way to communicate this.  

Also worth noting, you can change a PR back to 'draft' mode at any time (so 
whether you forgot initially, or a test failure or review feedback made you 
realize more work is needed, you can always amend the draft status to 
communicate whether you're still working, or ready for review again).

I'm not sure why you think this is going to make it more difficult to 
attract and retain new contributors.  The point is to make it easier for new 
contributors to get their PRs reviewed, and give reviewers more confident that 
their time is appreciated.

I see your point that as a committer, you have no need to communicate this 
status, since you can simply add reviewers yourself when you're ready.  Asking 
everyone to use draft status is not the ideal workaround for the GitHub flaw 
that non-committers can't request reviewers, but it also takes only 2 seconds 
of your time.  Expecting new contributors to email the dev list every time they 
want to request a review of their PR seems far more problematic in term of 
attracting and retaining new contributors.

On 10/28/20, 8:10 AM, "Bruce Schuchardt"  wrote:

-1
While I often use the Draft option I don't see why we want to add even 
more rules about how we use github.  I think it's enough to put in a PR and 
then add reviewers when you're ready for comments.  Getting the stink-eye for 
putting up a non-Draft PR is just going to make it more difficult to attract 
and retain new contributors.

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. 
As Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many 
PRs that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than 
we do on our own local machines. It is extremely uncommon to submit a PR the 
first time and have all tests go green. Which h means we invariably iterate 
over commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when 
the ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially 
submitted as a DRAFT PR. This way, all test can still be run and work can be 
done to make sure "green" is achieved. Once "green" status has been achieved, 
the draft can then be upgraded to a final PR by pressing the "Ready For Review" 
button. At this point all commits on the branch can then once again be squashed 
into a single commit.
Now project committers will now know that the PR is in a state that 
it can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting 
a PR monitors their PR for activity. If there is no activity for a few days, 
please feel free to ping the Apache Geode Dev community for review. If review 
is request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the 
time between review and response has been so long, that both parties (reviewer 
and submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than 
expected, please downgrade the PR to draft status again. At this point, it does 
not mean that reviewers will not be able to help anymore, as you can request a 
reviewer to help with feedback and comments, until 

Re: PR process and etiquette

[Anthony Baker]

I think exploring these additions to PR reviews would be pretty helpful.  It’s 
worth spending the extra time to get a PR right before merging.

Anthony

> On Oct 28, 2020, at 8:40 AM, Robert Houghton  wrote:
> 
> There are some pieces of Apache infrastructure we can control without needing 
> tickets: Specifically
> required_pull_request_reviews:
>dismiss_stale_reviews: true
>require_code_owner_reviews: true
> 
> I think these specific controls could go a long way towards helping keep our 
> PR quality high, while reducing cognitive load for the reviewers.
> 
> Full documentation 
> here
> 

Re: PR process and etiquette

[Owen Nichols]

Hey Bruce, please consider that non-committers are not permitted to add 
reviewers themselves, so a consistent convention to indicate when a PR has 
moved from work-in-progress to ready-for-review will help alert the community 
when to assign reviewers.

Currently, I see countless creative variations on people adding "WIP" or "DO 
NOT REVIEW" or other text somewhere in the summary or description.  I think the 
suggestion here is to standardize on using Draft status as the canonical way to 
communicate this.  

Also worth noting, you can change a PR back to 'draft' mode at any time (so 
whether you forgot initially, or a test failure or review feedback made you 
realize more work is needed, you can always amend the draft status to 
communicate whether you're still working, or ready for review again).

I'm not sure why you think this is going to make it more difficult to attract 
and retain new contributors.  The point is to make it easier for new 
contributors to get their PRs reviewed, and give reviewers more confident that 
their time is appreciated.

I see your point that as a committer, you have no need to communicate this 
status, since you can simply add reviewers yourself when you're ready.  Asking 
everyone to use draft status is not the ideal workaround for the GitHub flaw 
that non-committers can't request reviewers, but it also takes only 2 seconds 
of your time.  Expecting new contributors to email the dev list every time they 
want to request a review of their PR seems far more problematic in term of 
attracting and retaining new contributors.

On 10/28/20, 8:10 AM, "Bruce Schuchardt"  wrote:

-1
While I often use the Draft option I don't see why we want to add even more 
rules about how we use github.  I think it's enough to put in a PR and then add 
reviewers when you're ready for comments.  Getting the stink-eye for putting up 
a non-Draft PR is just going to make it more difficult to attract and retain 
new contributors.

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As 
Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs 
that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we 
do on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which h means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted 
as a DRAFT PR. This way, all test can still be run and work can be done to make 
sure "green" is achieved. Once "green" status has been achieved, the draft can 
then be upgraded to a final PR by pressing the "Ready For Review" button. At 
this point all commits on the branch can then once again be squashed into a 
single commit.
Now project committers will now know that the PR is in a state that it 
can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a 
PR monitors their PR for activity. If there is no activity for a few days, 
please feel free to ping the Apache Geode Dev community for review. If review 
is request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than 
expected, please downgrade the PR to draft status again. At this point, it does 
not mean that reviewers will not be able to help anymore, as you can request a 
reviewer to help with feedback and comments, until one feels that the PR is 
back in a state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community if there is no review on the PR. If feedback is provided on a PR, 
please address it as soon as possible. This not only will help the PR progress 
faster, but it will shorten the feedback loop.
Finally, start with a DRAFT PR and then upgrade to a final PR 
when you feel it is in a "good" state. If more work is required, it is ok to 

Re: PR process and etiquette

[Robert Houghton]

There are some pieces of Apache infrastructure we can control without needing 
tickets: Specifically
required_pull_request_reviews:
dismiss_stale_reviews: true
require_code_owner_reviews: true

I think these specific controls could go a long way towards helping keep our PR 
quality high, while reducing cognitive load for the reviewers.

Full documentation 
here



From: Bruce Schuchardt 
Date: Wednesday, October 28, 2020 at 8:10 AM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette
-1
While I often use the Draft option I don't see why we want to add even more 
rules about how we use github.  I think it's enough to put in a PR and then add 
reviewers when you're ready for comments.  Getting the stink-eye for putting up 
a non-Draft PR is just going to make it more difficult to attract and retain 
new contributors.

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As Josh 
Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs that 
have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we do 
on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which h means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted as a 
DRAFT PR. This way, all test can still be run and work can be done to make sure 
"green" is achieved. Once "green" status has been achieved, the draft can then 
be upgraded to a final PR by pressing the "Ready For Review" button. At this 
point all commits on the branch can then once again be squashed into a single 
commit.
Now project committers will now know that the PR is in a state that it can 
be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a PR 
monitors their PR for activity. If there is no activity for a few days, please 
feel free to ping the Apache Geode Dev community for review. If review is 
request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than expected, 
please downgrade the PR to draft status again. At this point, it does not mean 
that reviewers will not be able to help anymore, as you can request a reviewer 
to help with feedback and comments, until one feels that the PR is back in a 
state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community if there is no review on the PR. If feedback is provided on a PR, 
please address it as soon as possible. This not only will help the PR progress 
faster, but it will shorten the feedback loop.
Finally, start with a DRAFT PR and then upgrade to a final PR when 
you feel it is in a "good" state. If more work is required, it is ok to 
downgrade back to a draft, until one feels the PR is in a good state again.
Managing the PR process in this manner, will be hugely beneficial for all 
community members. As reviewers will know when a PR is ready to be reviewed. 
Reviewers will also feel more engaged in this process, due to timely response 
to their review comments. PR submitters will feel happier, in the knowledge 
that the code they spent so long meticulously crafting will possibly make it 
into the project.
Any thoughts?
--Udo

Re: How to add a new REST end point in Cluster Management Service

[John Blum]

Hi Jinmei-

I put feedback and other comments in the Wiki page.

Regards,
John


From: Jinmei Liao 
Sent: Monday, October 26, 2020 10:53 AM
To: dev@geode.apache.org 
Subject: How to add a new REST end point in Cluster Management Service

Hi, Geode developers,

I just added a twiki page detailing how to add a new REST end point in Cluster 
Management Service to manage new entities or start new long running operations 
in Geode Cluster. Feel free to look it through and provide comments/suggestions.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FAdd%2BA%2BREST%2BEnd%2Bpoint%2Bto%2BCluster%2BManagement%2BServicedata=04%7C01%7Cjblum%40vmware.com%7C302d5e04c4a740848b3c08d879d81af6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637393316392029047%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=KSiNyaHOptRWPRP5bbRiFbT50a5fDG%2FRd7IuoRZGiQQ%3Dreserved=0

Thanks!

Jinmei

Re: PR process and etiquette

[Bruce Schuchardt]

-1
While I often use the Draft option I don't see why we want to add even more 
rules about how we use github.  I think it's enough to put in a PR and then add 
reviewers when you're ready for comments.  Getting the stink-eye for putting up 
a non-Draft PR is just going to make it more difficult to attract and retain 
new contributors.

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As Josh 
Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs that 
have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we do 
on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which h means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted as a 
DRAFT PR. This way, all test can still be run and work can be done to make sure 
"green" is achieved. Once "green" status has been achieved, the draft can then 
be upgraded to a final PR by pressing the "Ready For Review" button. At this 
point all commits on the branch can then once again be squashed into a single 
commit.
Now project committers will now know that the PR is in a state that it can 
be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a PR 
monitors their PR for activity. If there is no activity for a few days, please 
feel free to ping the Apache Geode Dev community for review. If review is 
request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than expected, 
please downgrade the PR to draft status again. At this point, it does not mean 
that reviewers will not be able to help anymore, as you can request a reviewer 
to help with feedback and comments, until one feels that the PR is back in a 
state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community if there is no review on the PR. If feedback is provided on a PR, 
please address it as soon as possible. This not only will help the PR progress 
faster, but it will shorten the feedback loop.
Finally, start with a DRAFT PR and then upgrade to a final PR when 
you feel it is in a "good" state. If more work is required, it is ok to 
downgrade back to a draft, until one feels the PR is in a good state again.
Managing the PR process in this manner, will be hugely beneficial for all 
community members. As reviewers will know when a PR is ready to be reviewed. 
Reviewers will also feel more engaged in this process, due to timely response 
to their review comments. PR submitters will feel happier, in the knowledge 
that the code they spent so long meticulously crafting will possibly make it 
into the project.
Any thoughts?
--Udo

Re: PR process and etiquette

[Blake Bender]

Oops, sorry for the confusion!  I’ve been working through Mario’s PRs a lot 
lately.


From: Alberto Gomez 
Date: Wednesday, October 28, 2020 at 7:10 AM
To: "dev@geode.apache.org" , Blake Bender 

Subject: Re: PR process and etiquette

+1 to draft PRs.

By the way @Blake Bender, it's me the one having the 
draft PR for GEODE-8318.

Alberto G.

From: Blake Bender 
Sent: Wednesday, October 28, 2020 2:28 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

+1 for draft PRs.  Native has been using these for a few months now, and 
they're quite effective.  Right now, for example, we have 6 PRs up, 3 of which 
are draft.  They also turn out to be a convenient way to share work, in certain 
circumstances.  Mario, for instance, has a draft up for GEODE-8318 that is 
strictly WIP.  By having it up as a draft PR, I get notifications when changes 
are pushed, and can run internal tooling and let him know if I find issues.

Thanks,

Blake


On 10/28/20, 1:03 AM, "Udo Kohlmeyer"  wrote:

Great information Darrel. Thank you for sharing that.

--Udo

From: Darrel Schneider 
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to 
be aware of is that if your pr branch has conflicts and it is in draft mode 
then your pr tests will not run and the pr page will not tell you that 
conflicts exist. If you see that the pr tests are not actually running and it 
is in draft mode then try merging develop to your pr branch and resolve the 
conflicts.

From: Owen Nichols 
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

+1 for using GitHub's draft status to indicate work-in-progress.

Many great suggestions here, however I generally prefer that we don't 
squash commits at any point except the final Squash and Merge to develop.  I 
find it insightful to see how the work evolved.  I also find that review 
comments may start coming in even before you are "ready" for review, and a 
squash or force-push "loses" those comments.

One thing I would like to see more of is PR summaries that explain *why* 
the change is being made, not just *what* is being changed.

Thanks Udo for looking for ways to make the community process work even 
better!

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As 
Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs 
that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we 
do on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted 
as a DRAFT PR. This way, all test can still be run and work can be done to make 
sure "green" is achieved. Once "green" status has been achieved, the draft can 
then be upgraded to a final PR by pressing the "Ready For Review" button. At 
this point all commits on the branch can then once again be squashed into a 
single commit.
Now project committers will now know that the PR is in a state that it 
can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a 
PR monitors their PR for activity. If there is no activity for a few days, 
please feel free to ping the Apache Geode Dev community for review. If review 
is request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than 
expected, please downgrade the PR to draft status again. At this point, it does 
not mean that reviewers will not be able to help anymore, as you can request a 
reviewer to help with feedback and comments, until one feels that the PR is 
back in a state of final 

Re: PR process and etiquette

[Alberto Gomez]

+1 to draft PRs.

By the way @Blake Bender, it's me the one having the 
draft PR for GEODE-8318.

Alberto G.

From: Blake Bender 
Sent: Wednesday, October 28, 2020 2:28 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

+1 for draft PRs.  Native has been using these for a few months now, and 
they're quite effective.  Right now, for example, we have 6 PRs up, 3 of which 
are draft.  They also turn out to be a convenient way to share work, in certain 
circumstances.  Mario, for instance, has a draft up for GEODE-8318 that is 
strictly WIP.  By having it up as a draft PR, I get notifications when changes 
are pushed, and can run internal tooling and let him know if I find issues.

Thanks,

Blake


On 10/28/20, 1:03 AM, "Udo Kohlmeyer"  wrote:

Great information Darrel. Thank you for sharing that.

--Udo

From: Darrel Schneider 
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to 
be aware of is that if your pr branch has conflicts and it is in draft mode 
then your pr tests will not run and the pr page will not tell you that 
conflicts exist. If you see that the pr tests are not actually running and it 
is in draft mode then try merging develop to your pr branch and resolve the 
conflicts.

From: Owen Nichols 
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

+1 for using GitHub's draft status to indicate work-in-progress.

Many great suggestions here, however I generally prefer that we don't 
squash commits at any point except the final Squash and Merge to develop.  I 
find it insightful to see how the work evolved.  I also find that review 
comments may start coming in even before you are "ready" for review, and a 
squash or force-push "loses" those comments.

One thing I would like to see more of is PR summaries that explain *why* 
the change is being made, not just *what* is being changed.

Thanks Udo for looking for ways to make the community process work even 
better!

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As 
Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs 
that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we 
do on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted 
as a DRAFT PR. This way, all test can still be run and work can be done to make 
sure "green" is achieved. Once "green" status has been achieved, the draft can 
then be upgraded to a final PR by pressing the "Ready For Review" button. At 
this point all commits on the branch can then once again be squashed into a 
single commit.
Now project committers will now know that the PR is in a state that it 
can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a 
PR monitors their PR for activity. If there is no activity for a few days, 
please feel free to ping the Apache Geode Dev community for review. If review 
is request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than 
expected, please downgrade the PR to draft status again. At this point, it does 
not mean that reviewers will not be able to help anymore, as you can request a 
reviewer to help with feedback and comments, until one feels that the PR is 
back in a state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community if there is no review on the PR. If feedback is provided on a PR, 
please 

Re: PR process and etiquette

[Blake Bender]

+1 for draft PRs.  Native has been using these for a few months now, and 
they're quite effective.  Right now, for example, we have 6 PRs up, 3 of which 
are draft.  They also turn out to be a convenient way to share work, in certain 
circumstances.  Mario, for instance, has a draft up for GEODE-8318 that is 
strictly WIP.  By having it up as a draft PR, I get notifications when changes 
are pushed, and can run internal tooling and let him know if I find issues.

Thanks,

Blake


On 10/28/20, 1:03 AM, "Udo Kohlmeyer"  wrote:

Great information Darrel. Thank you for sharing that.

--Udo

From: Darrel Schneider 
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to 
be aware of is that if your pr branch has conflicts and it is in draft mode 
then your pr tests will not run and the pr page will not tell you that 
conflicts exist. If you see that the pr tests are not actually running and it 
is in draft mode then try merging develop to your pr branch and resolve the 
conflicts.

From: Owen Nichols 
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

+1 for using GitHub's draft status to indicate work-in-progress.

Many great suggestions here, however I generally prefer that we don't 
squash commits at any point except the final Squash and Merge to develop.  I 
find it insightful to see how the work evolved.  I also find that review 
comments may start coming in even before you are "ready" for review, and a 
squash or force-push "loses" those comments.

One thing I would like to see more of is PR summaries that explain *why* 
the change is being made, not just *what* is being changed.

Thanks Udo for looking for ways to make the community process work even 
better!

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As 
Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs 
that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we 
do on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted 
as a DRAFT PR. This way, all test can still be run and work can be done to make 
sure "green" is achieved. Once "green" status has been achieved, the draft can 
then be upgraded to a final PR by pressing the "Ready For Review" button. At 
this point all commits on the branch can then once again be squashed into a 
single commit.
Now project committers will now know that the PR is in a state that it 
can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a 
PR monitors their PR for activity. If there is no activity for a few days, 
please feel free to ping the Apache Geode Dev community for review. If review 
is request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than 
expected, please downgrade the PR to draft status again. At this point, it does 
not mean that reviewers will not be able to help anymore, as you can request a 
reviewer to help with feedback and comments, until one feels that the PR is 
back in a state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community if there is no review on the PR. If feedback is provided on a PR, 
please address it as soon as possible. This not only will help the PR progress 
faster, but it will shorten the feedback loop.
Finally, start with a DRAFT PR and then upgrade to a final PR 
when you feel it is in a "good" state. If more work is required, it is ok to 
downgrade back to a 

Re: PR process and etiquette

[Udo Kohlmeyer]

Great information Darrel. Thank you for sharing that.

--Udo

From: Darrel Schneider 
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to be 
aware of is that if your pr branch has conflicts and it is in draft mode then 
your pr tests will not run and the pr page will not tell you that conflicts 
exist. If you see that the pr tests are not actually running and it is in draft 
mode then try merging develop to your pr branch and resolve the conflicts.

From: Owen Nichols 
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org 
Subject: Re: PR process and etiquette

+1 for using GitHub's draft status to indicate work-in-progress.

Many great suggestions here, however I generally prefer that we don't squash 
commits at any point except the final Squash and Merge to develop.  I find it 
insightful to see how the work evolved.  I also find that review comments may 
start coming in even before you are "ready" for review, and a squash or 
force-push "loses" those comments.

One thing I would like to see more of is PR summaries that explain *why* the 
change is being made, not just *what* is being changed.

Thanks Udo for looking for ways to make the community process work even better!

On 10/27/20, 5:41 PM, "Udo Kohlmeyer"  wrote:

Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted. As Josh 
Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many PRs that 
have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than we do 
on our own local machines. It is extremely uncommon to submit a PR the first 
time and have all tests go green. Which means we invariably iterate over 
commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when the 
ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially submitted as a 
DRAFT PR. This way, all test can still be run and work can be done to make sure 
"green" is achieved. Once "green" status has been achieved, the draft can then 
be upgraded to a final PR by pressing the "Ready For Review" button. At this 
point all commits on the branch can then once again be squashed into a single 
commit.
Now project committers will now know that the PR is in a state that it can 
be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting a PR 
monitors their PR for activity. If there is no activity for a few days, please 
feel free to ping the Apache Geode Dev community for review. If review is 
request, please prioritize some time to address the feedback, as reviewers 
spend time reviewing code and getting an understanding what the code is doing. 
If too much time goes by, between review and addressing the review comments, 
not only does the reviewer lose context, possibly requiring them to spend time 
again to understand what the code was/is supposed to do, but also possibly lose 
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the time 
between review and response has been so long, that both parties (reviewer and 
submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than expected, 
please downgrade the PR to draft status again. At this point, it does not mean 
that reviewers will not be able to help anymore, as you can request a reviewer 
to help with feedback and comments, until one feels that the PR is back in a 
state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the 
community if there is no review on the PR. If feedback is provided on a PR, 
please address it as soon as possible. This not only will help the PR progress 
faster, but it will shorten the feedback loop.
Finally, start with a DRAFT PR and then upgrade to a final PR when 
you feel it is in a "good" state. If more work is required, it is ok to 
downgrade back to a draft, until one feels the PR is in a good state again.
Managing the PR process in this manner, will be hugely beneficial for all 
community members. As reviewers will know when a PR is ready to be reviewed. 
Reviewers will also feel more engaged in this process, due to timely response 
to their review comments. PR submitters will feel happier, in the knowledge 
that the code they spent so long meticulously crafting will possibly make it 
into the project.
Any thoughts?
--Udo