Re: PR process and etiquette

[Darrel Schneider]

+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

Re: PR process and etiquette

[Udo Kohlmeyer]

Great ideas Owen.

I do apologize for the BIG lump of text… stupid formatting of lack thereof…

--Udo

From: Owen Nichols 
Date: Wednesday, October 28, 2020 at 12: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

Re: PR process and etiquette

[Owen Nichols]

+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

PR process and etiquette

[Udo Kohlmeyer]

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

Re: [PROPOSAL] backport GEODE-8651 to 1.13, 9.10, 9.9

[Jinmei Liao]

+1

On Oct 27, 2020 3:00 PM, Donal Evans  wrote:
+1

From: Anthony Baker 
Sent: Tuesday, October 27, 2020 2:53 PM
To: dev@geode.apache.org 
Subject: Re: [PROPOSAL] backport GEODE-8651 to 1.13, 9.10, 9.9

+1 from me

> On Oct 27, 2020, at 2:21 PM, Xiaojian Zhou  wrote:
>
> Hi, all:
>
> The fix is to resolve a hang when Connection called notifyHandshakeWaiter the 
> 2nd time and cleared the NioFilter’s unwrapped buffer by mistake.
>
> The 2nd call should consider if the 1st call has finished handshake. If yes, 
> do nothing. The fix is fully tested and has no risk. This problem exists in 
> earlier versions and should be backported.
>
> Regards
>
> Xiaojian Zhou

Re: [PROPOSAL] backport GEODE-8651 to 1.13, 9.10, 9.9

[Donal Evans]

+1

From: Anthony Baker 
Sent: Tuesday, October 27, 2020 2:53 PM
To: dev@geode.apache.org 
Subject: Re: [PROPOSAL] backport GEODE-8651 to 1.13, 9.10, 9.9

+1 from me

> On Oct 27, 2020, at 2:21 PM, Xiaojian Zhou  wrote:
>
> Hi, all:
>
> The fix is to resolve a hang when Connection called notifyHandshakeWaiter the 
> 2nd time and cleared the NioFilter’s unwrapped buffer by mistake.
>
> The 2nd call should consider if the 1st call has finished handshake. If yes, 
> do nothing. The fix is fully tested and has no risk. This problem exists in 
> earlier versions and should be backported.
>
> Regards
>
> Xiaojian Zhou

Re: [PROPOSAL] backport GEODE-8651 to 1.13, 9.10, 9.9

[Anthony Baker]

+1 from me

> On Oct 27, 2020, at 2:21 PM, Xiaojian Zhou  wrote:
> 
> Hi, all:
> 
> The fix is to resolve a hang when Connection called notifyHandshakeWaiter the 
> 2nd time and cleared the NioFilter’s unwrapped buffer by mistake.
> 
> The 2nd call should consider if the 1st call has finished handshake. If yes, 
> do nothing. The fix is fully tested and has no risk. This problem exists in 
> earlier versions and should be backported.
> 
> Regards
> 
> Xiaojian Zhou

Please review PR #5667

[Kirk Lund]

GEODE-8647: Support multiple instances of DistributedMap #5667
https://github.com/apache/geode/pull/5667

Thanks,
Kirk

[PROPOSAL] backport GEODE-8651 to 1.13, 9.10, 9.9

[Xiaojian Zhou]

Hi, all:

The fix is to resolve a hang when Connection called notifyHandshakeWaiter the 
2nd time and cleared the NioFilter’s unwrapped buffer by mistake.

The 2nd call should consider if the 1st call has finished handshake. If yes, do 
nothing. The fix is fully tested and has no risk. This problem exists in 
earlier versions and should be backported.

Regards

Xiaojian Zhou

Re: PRs to review in geode-native

[Mario Salazar de Torres]

Hi Blake,

I am hoping opening a PR to support my initial test proposal is off the table, 
right? If so, give me a couple of days and I will think of alternatives.
If nothing comes to my mind, then I'll reach you back.

Thanks btw for merging PR #667

BR,
Mario.

From: Blake Bender 
Sent: Tuesday, October 27, 2020 6:25 PM
To: dev@geode.apache.org 
Subject: Re: PRs to review in geode-native

I've asked mreddington to re-review today.  I think we're still hoping to have 
a test for this, though, right?

On 10/27/20, 5:51 AM, "Mario Salazar de Torres" 
 wrote:

Hi everyone,

Thanks everyone involved for merging PR 
#659.
Thing is that it has been a while since 
#660
 got any feedback.
Is there any chance you could reserve some time to give me some feedback on 
it?

Thanks for all.

BR,
Mario.


From: Blake Bender 
Sent: Wednesday, September 30, 2020 4:34 PM
To: dev@geode.apache.org 
Subject: Re: PRs to review in geode-native

We'll get on these today, no worries.

Thanks,

Blake


On 9/30/20, 2:08 AM, "Mario Salazar de Torres" 
 wrote:

Hi everyone,

I've created 2 PR in geode-native repository:

  *   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F659data=04%7C01%7Cbblake%40vmware.com%7C638426e25e644d6ad82508d87a76f590%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637393998640926274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=%2F3gELhN0B2XBC5pngQv1A0WLhaCoenMZVnCFaUOM7oM%3Dreserved=0
  *   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F660data=04%7C01%7Cbblake%40vmware.com%7C638426e25e644d6ad82508d87a76f590%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637393998640926274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=59wr1HMHQhNOvMczV%2FNeq5yDo1n1jz6jDQOD%2BNlXCXI%3Dreserved=0

If any of you could take some time to throw some comments on them, I 
would be so much appreciated.
PST. It's my first time contributing to the project, so please don't be 
too mean with me ha-ha 

Thanks!
BR/Mario

Re: PRs to review in geode-native

[Blake Bender]

I've asked mreddington to re-review today.  I think we're still hoping to have 
a test for this, though, right?  

On 10/27/20, 5:51 AM, "Mario Salazar de Torres" 
 wrote:

Hi everyone,

Thanks everyone involved for merging PR 
#659.
Thing is that it has been a while since 
#660
 got any feedback.
Is there any chance you could reserve some time to give me some feedback on 
it?

Thanks for all.

BR,
Mario.


From: Blake Bender 
Sent: Wednesday, September 30, 2020 4:34 PM
To: dev@geode.apache.org 
Subject: Re: PRs to review in geode-native

We'll get on these today, no worries.

Thanks,

Blake


On 9/30/20, 2:08 AM, "Mario Salazar de Torres" 
 wrote:

Hi everyone,

I've created 2 PR in geode-native repository:

  *   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F659data=04%7C01%7Cbblake%40vmware.com%7C638426e25e644d6ad82508d87a76f590%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637393998640926274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=%2F3gELhN0B2XBC5pngQv1A0WLhaCoenMZVnCFaUOM7oM%3Dreserved=0
  *   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F660data=04%7C01%7Cbblake%40vmware.com%7C638426e25e644d6ad82508d87a76f590%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637393998640926274%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=59wr1HMHQhNOvMczV%2FNeq5yDo1n1jz6jDQOD%2BNlXCXI%3Dreserved=0

If any of you could take some time to throw some comments on them, I 
would be so much appreciated.
PST. It's my first time contributing to the project, so please don't be 
too mean with me ha-ha 

Thanks!
BR/Mario

Re: PRs to review in geode-native

[Mario Salazar de Torres]

Hi everyone,

Thanks everyone involved for merging PR 
#659.
Thing is that it has been a while since 
#660 got any feedback.
Is there any chance you could reserve some time to give me some feedback on it?

Thanks for all.

BR,
Mario.


From: Blake Bender 
Sent: Wednesday, September 30, 2020 4:34 PM
To: dev@geode.apache.org 
Subject: Re: PRs to review in geode-native

We'll get on these today, no worries.

Thanks,

Blake


On 9/30/20, 2:08 AM, "Mario Salazar de Torres" 
 wrote:

Hi everyone,

I've created 2 PR in geode-native repository:

  *   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F659data=02%7C01%7Cbblake%40vmware.com%7Cdc2d4126f8404c610e7f08d865206ef0%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637370537265990061sdata=T7mRZ6%2B01yd6wOUmbE2s2Q%2FWAOM63E9ZE%2FSBbrB1wmk%3Dreserved=0
  *   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F660data=02%7C01%7Cbblake%40vmware.com%7Cdc2d4126f8404c610e7f08d865206ef0%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637370537265990061sdata=%2F0XPvKcup8OcgnG8bfvJ1b0ZTkJTb4IztbsooCsKto0%3Dreserved=0

If any of you could take some time to throw some comments on them, I would 
be so much appreciated.
PST. It's my first time contributing to the project, so please don't be too 
mean with me ha-ha 

Thanks!
BR/Mario