Proposal to including GEODE-7593 in release/1.11.0

[Owen Nichols]

GEODE-7593 fixes a memory leak where indexes could retain references to pdx 
values when eviction should have released that memory.

This is not a new issue, but is critical because system stability is threatened 
when eviction does not release memory as expected.

The SHA is 1beec9e3930a071031b960f045874fb337e72e7c.

Re: Proposal to including GEODE-7593 in release/1.11.0

[Jason Huynh]

+1

On Thu, Dec 19, 2019 at 10:05 AM Owen Nichols  wrote:

> GEODE-7593 fixes a memory leak where indexes could retain references to
> pdx values when eviction should have released that memory.
>
> This is not a new issue, but is critical because system stability is
> threatened when eviction does not release memory as expected.
>
> The SHA is 1beec9e3930a071031b960f045874fb337e72e7c.

Re: Proposal to including GEODE-7593 in release/1.11.0

[Udo Kohlmeyer]

+1

On 12/19/19 10:05 AM, Owen Nichols wrote:

GEODE-7593 fixes a memory leak where indexes could retain references to pdx 
values when eviction should have released that memory.

This is not a new issue, but is critical because system stability is threatened 
when eviction does not release memory as expected.

The SHA is 1beec9e3930a071031b960f045874fb337e72e7c.

Re: Proposal to including GEODE-7593 in release/1.11.0

[Dick Cavender]

+1

On Thu, Dec 19, 2019 at 11:27 AM Udo Kohlmeyer  wrote:

> +1
>
> On 12/19/19 10:05 AM, Owen Nichols wrote:
> > GEODE-7593 fixes a memory leak where indexes could retain references to
> pdx values when eviction should have released that memory.
> >
> > This is not a new issue, but is critical because system stability is
> threatened when eviction does not release memory as expected.
> >
> > The SHA is 1beec9e3930a071031b960f045874fb337e72e7c.
>

[DISCUSS] Does anyone know of any other issues for 1.11.0?

[Mark Hanson]

Hi All,

I believe at this point, that all outstanding issues have been included in 
1.11.0. I would like to do an RC build unless someone advises me otherwise, by 
3pm PST. Everyone will still have a few days to try the release candidate, as 
usual.

Thanks,
Mark

Re: Proposal to including GEODE-7593 in release/1.11.0

[Ivan Godwin]

+1

On Thu, Dec 19, 2019 at 11:43 AM Dick Cavender  wrote:

> +1
>
> On Thu, Dec 19, 2019 at 11:27 AM Udo Kohlmeyer  wrote:
>
> > +1
> >
> > On 12/19/19 10:05 AM, Owen Nichols wrote:
> > > GEODE-7593 fixes a memory leak where indexes could retain references to
> > pdx values when eviction should have released that memory.
> > >
> > > This is not a new issue, but is critical because system stability is
> > threatened when eviction does not release memory as expected.
> > >
> > > The SHA is 1beec9e3930a071031b960f045874fb337e72e7c.
> >
>

Re: Proposal to including GEODE-7593 in release/1.11.0

[Mark Hanson]

I have added the SHA to the branch.

Thanks,
Mark

> On Dec 19, 2019, at 12:25 PM, Ivan Godwin  wrote:
> 
> +1
> 
> On Thu, Dec 19, 2019 at 11:43 AM Dick Cavender  wrote:
> 
>> +1
>> 
>> On Thu, Dec 19, 2019 at 11:27 AM Udo Kohlmeyer  wrote:
>> 
>>> +1
>>> 
>>> On 12/19/19 10:05 AM, Owen Nichols wrote:
 GEODE-7593 fixes a memory leak where indexes could retain references to
>>> pdx values when eviction should have released that memory.
 
 This is not a new issue, but is critical because system stability is
>>> threatened when eviction does not release memory as expected.
 
 The SHA is 1beec9e3930a071031b960f045874fb337e72e7c.
>>> 
>> 

[DISCUSS] Proposal to require linear commit history on develop

[Owen Nichols]

I’d like to advance this topic from an informal request/discussion to a 
discussion of a concrete proposal.

To recap, it sounds like there is general agreement that commit history on 
develop should be linear (no merge commits), and that the biggest obstacle to 
this is that the PR merge button in GitHub creates a merge commit by default.

I propose the following changes:
1) Change GitHub settings to remove the ability to create a merge commit.  This 
will make Squash & Merge the default.

2) Change GitHub settings to require linear history on develop.  This prevents 
merge commits via command-line (not recommended, but wiki still has 
instructions for merging PRs this way).

3) Update the PR template to change the text "Is your initial contribution a 
single, squashed commit” to “Is your initial contribution squashed for ease of 
review (e.g. a single commit, or a ‘refactoring’ commit plus a ‘fix’ commit)"

For clarity, I am proposing no-change in the following areas:
i) Leave Rebase & Merge as an option for PRs that have been structured to 
benefit from it (this can make it easier in a bisect to see whether the 
refactoring or the “fix” broke something).
ii) Leave existing wording in the wiki as-is [stating that squashing is 
preferred].


Please comment via this email thread. 
-Owen



> On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
> 
> I think it's already resolved Udo ;)
> 
> Here's the problem, if I fixup a dunit test by removing all uses of "this."
> and I rename the dunit test, then git doesn't remember that the file is a
> rename -- it forever afterwards interprets it as a new file that I created
> if I touch more than 50% of the lines (which "this." can easily do). If we
> squash two commits: the rename and the cleanup of that dunit test -- then
> we effectively lose the history of that file and it shows that I created a
> new file.
> 
> Also for the record, I've been working on Geode since the beginning and I
> was never made aware of this change in our process. I never voted on it.
> I'm not a big fan of changing various details in our process every single
> week. It's very easy to miss these discussions unless someone points it out
> to me.
> 
> On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
> wrote:
> 
>> I'm not sure what this discussion is about... WE, as a community, have
>> agreed in common practices, in two place no less...
>> 
>> 1) Quoting our PR template
>> 
>> 
>>  For all changes:
>> 
>>  *
>> 
>>Is there a JIRA ticket associated with this PR? Is it referenced in
>>the commit message?
>> 
>>  *
>> 
>>Has your PR been rebased against the latest commit within the target
>>branch (typically|develop|)?
>> 
>>  *
>> 
>>***Is your initial contribution a single, squashed commit?*
>> 
>>  *
>> 
>>Does|gradlew build|run cleanly?
>> 
>>  *
>> 
>>Have you written or updated unit tests to verify your changes?
>> 
>>  *
>> 
>>If adding new dependencies to the code, are these dependencies
>>licensed in a way that is compatible for inclusion underASF 2.0
>>?
>> 
>> On our PR template we call out that the initial PR commit should be
>> squashed.
>> 
>> 2)https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
>> 
>> -- See "Accepting a PR Using the Command Line" - Point #3.
>> 
>> 
>> @Kirk, if each of your commits "stands alone", I commend you on the
>> diligence, but in reality, they should either then be stand alone PR's
>> or just extra work created for yourself.
>> 
>> If we want to change the way we have agreed upon we submit/commit/merge
>> changes back into develop... Then this is another discussion thread,
>> until then, I think we should all remind ourselves on our agreed
>> contributions code of conduct.
>> 
>> --Udo
>> 
>> On 12/16/19 9:59 AM, Nabarun Nag wrote:
>>> Kirk, I believe that creating a Pull Request with multiple commits is ok.
>>> It's just in the end that when it's being pushed to develop branch, it
>>> needs to be squash merged. I believe that is what you have mentioned in
>> the
>>> first paragraph, and I am more than happy with that.
>>> If you can see in the first screenshot comparison that I had attached in
>>> the first email in this thread is what I want to avoid.
>>> 
>>> Thank you for your feedback.
>>> 
>>> Regards
>>> Naba
>>> 
>>> 
>>> On Mon, Dec 16, 2019 at 9:47 AM Kirk Lund  wrote:
>>> 
 Whenever I submit a PR with multiple commits that I intend to rebase to
 develop, I always try to ensure that each commit stands alone as is
 (compiles and passes tests). Separating file renames and refactoring
>> from
 behavior changes into different commits seems very valuable to me, and
>> I've
 had trouble getting people to review PRs without this separation (but it
 could be squashed as it's merged to develop).
 
 It sounds to me like the real problem is (a

Re: [DISCUSS] Proposal to require linear commit history on develop

[Jinmei Liao]

+1

On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:

> I’d like to advance this topic from an informal request/discussion to a
> discussion of a concrete proposal.
>
> To recap, it sounds like there is general agreement that commit history on
> develop should be linear (no merge commits), and that the biggest obstacle
> to this is that the PR merge button in GitHub creates a merge commit by
> default.
>
> I propose the following changes:
> 1) Change GitHub settings to remove the ability to create a merge commit.
> This will make Squash & Merge the default.
>
> 2) Change GitHub settings to require linear history on develop.  This
> prevents merge commits via command-line (not recommended, but wiki still
> has instructions for merging PRs this way).
>
> 3) Update the PR template to change the text "Is your initial contribution
> a single, squashed commit” to “Is your initial contribution squashed for
> ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
> ‘fix’ commit)"
>
> For clarity, I am proposing no-change in the following areas:
> i) Leave Rebase & Merge as an option for PRs that have been structured to
> benefit from it (this can make it easier in a bisect to see whether the
> refactoring or the “fix” broke something).
> ii) Leave existing wording in the wiki as-is [stating that squashing is
> preferred].
>
>
> Please comment via this email thread.
> -Owen
>
>
>
> > On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
> >
> > I think it's already resolved Udo ;)
> >
> > Here's the problem, if I fixup a dunit test by removing all uses of
> "this."
> > and I rename the dunit test, then git doesn't remember that the file is a
> > rename -- it forever afterwards interprets it as a new file that I
> created
> > if I touch more than 50% of the lines (which "this." can easily do). If
> we
> > squash two commits: the rename and the cleanup of that dunit test -- then
> > we effectively lose the history of that file and it shows that I created
> a
> > new file.
> >
> > Also for the record, I've been working on Geode since the beginning and I
> > was never made aware of this change in our process. I never voted on it.
> > I'm not a big fan of changing various details in our process every single
> > week. It's very easy to miss these discussions unless someone points it
> out
> > to me.
> >
> > On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
> > wrote:
> >
> >> I'm not sure what this discussion is about... WE, as a community, have
> >> agreed in common practices, in two place no less...
> >>
> >> 1) Quoting our PR template
> >>
> >>
> >>  For all changes:
> >>
> >>  *
> >>
> >>Is there a JIRA ticket associated with this PR? Is it referenced in
> >>the commit message?
> >>
> >>  *
> >>
> >>Has your PR been rebased against the latest commit within the target
> >>branch (typically|develop|)?
> >>
> >>  *
> >>
> >>***Is your initial contribution a single, squashed commit?*
> >>
> >>  *
> >>
> >>Does|gradlew build|run cleanly?
> >>
> >>  *
> >>
> >>Have you written or updated unit tests to verify your changes?
> >>
> >>  *
> >>
> >>If adding new dependencies to the code, are these dependencies
> >>licensed in a way that is compatible for inclusion underASF 2.0
> >>?
> >>
> >> On our PR template we call out that the initial PR commit should be
> >> squashed.
> >>
> >> 2)https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
> >> 
> >> -- See "Accepting a PR Using the Command Line" - Point #3.
> >>
> >>
> >> @Kirk, if each of your commits "stands alone", I commend you on the
> >> diligence, but in reality, they should either then be stand alone PR's
> >> or just extra work created for yourself.
> >>
> >> If we want to change the way we have agreed upon we submit/commit/merge
> >> changes back into develop... Then this is another discussion thread,
> >> until then, I think we should all remind ourselves on our agreed
> >> contributions code of conduct.
> >>
> >> --Udo
> >>
> >> On 12/16/19 9:59 AM, Nabarun Nag wrote:
> >>> Kirk, I believe that creating a Pull Request with multiple commits is
> ok.
> >>> It's just in the end that when it's being pushed to develop branch, it
> >>> needs to be squash merged. I believe that is what you have mentioned in
> >> the
> >>> first paragraph, and I am more than happy with that.
> >>> If you can see in the first screenshot comparison that I had attached
> in
> >>> the first email in this thread is what I want to avoid.
> >>>
> >>> Thank you for your feedback.
> >>>
> >>> Regards
> >>> Naba
> >>>
> >>>
> >>> On Mon, Dec 16, 2019 at 9:47 AM Kirk Lund  wrote:
> >>>
>  Whenever I submit a PR with multiple commits that I intend to rebase
> to
>  develop, I always try to ensure that each commit stands alone as is
>  (compiles and passes tests). Separating file renames and refactorin

Re: [DISCUSS] Proposal to require linear commit history on develop

[Mark Hanson]

+1

> On Dec 19, 2019, at 4:27 PM, Jinmei Liao  wrote:
> 
> +1
> 
> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:
> 
>> I’d like to advance this topic from an informal request/discussion to a
>> discussion of a concrete proposal.
>> 
>> To recap, it sounds like there is general agreement that commit history on
>> develop should be linear (no merge commits), and that the biggest obstacle
>> to this is that the PR merge button in GitHub creates a merge commit by
>> default.
>> 
>> I propose the following changes:
>> 1) Change GitHub settings to remove the ability to create a merge commit.
>> This will make Squash & Merge the default.
>> 
>> 2) Change GitHub settings to require linear history on develop.  This
>> prevents merge commits via command-line (not recommended, but wiki still
>> has instructions for merging PRs this way).
>> 
>> 3) Update the PR template to change the text "Is your initial contribution
>> a single, squashed commit” to “Is your initial contribution squashed for
>> ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
>> ‘fix’ commit)"
>> 
>> For clarity, I am proposing no-change in the following areas:
>> i) Leave Rebase & Merge as an option for PRs that have been structured to
>> benefit from it (this can make it easier in a bisect to see whether the
>> refactoring or the “fix” broke something).
>> ii) Leave existing wording in the wiki as-is [stating that squashing is
>> preferred].
>> 
>> 
>> Please comment via this email thread.
>> -Owen
>> 
>> 
>> 
>>> On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
>>> 
>>> I think it's already resolved Udo ;)
>>> 
>>> Here's the problem, if I fixup a dunit test by removing all uses of
>> "this."
>>> and I rename the dunit test, then git doesn't remember that the file is a
>>> rename -- it forever afterwards interprets it as a new file that I
>> created
>>> if I touch more than 50% of the lines (which "this." can easily do). If
>> we
>>> squash two commits: the rename and the cleanup of that dunit test -- then
>>> we effectively lose the history of that file and it shows that I created
>> a
>>> new file.
>>> 
>>> Also for the record, I've been working on Geode since the beginning and I
>>> was never made aware of this change in our process. I never voted on it.
>>> I'm not a big fan of changing various details in our process every single
>>> week. It's very easy to miss these discussions unless someone points it
>> out
>>> to me.
>>> 
>>> On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
>>> wrote:
>>> 
 I'm not sure what this discussion is about... WE, as a community, have
 agreed in common practices, in two place no less...
 
 1) Quoting our PR template
 
 
 For all changes:
 
 *
 
   Is there a JIRA ticket associated with this PR? Is it referenced in
   the commit message?
 
 *
 
   Has your PR been rebased against the latest commit within the target
   branch (typically|develop|)?
 
 *
 
   ***Is your initial contribution a single, squashed commit?*
 
 *
 
   Does|gradlew build|run cleanly?
 
 *
 
   Have you written or updated unit tests to verify your changes?
 
 *
 
   If adding new dependencies to the code, are these dependencies
   licensed in a way that is compatible for inclusion underASF 2.0
   ?
 
 On our PR template we call out that the initial PR commit should be
 squashed.
 
 2)https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
 
 -- See "Accepting a PR Using the Command Line" - Point #3.
 
 
 @Kirk, if each of your commits "stands alone", I commend you on the
 diligence, but in reality, they should either then be stand alone PR's
 or just extra work created for yourself.
 
 If we want to change the way we have agreed upon we submit/commit/merge
 changes back into develop... Then this is another discussion thread,
 until then, I think we should all remind ourselves on our agreed
 contributions code of conduct.
 
 --Udo
 
 On 12/16/19 9:59 AM, Nabarun Nag wrote:
> Kirk, I believe that creating a Pull Request with multiple commits is
>> ok.
> It's just in the end that when it's being pushed to develop branch, it
> needs to be squash merged. I believe that is what you have mentioned in
 the
> first paragraph, and I am more than happy with that.
> If you can see in the first screenshot comparison that I had attached
>> in
> the first email in this thread is what I want to avoid.
> 
> Thank you for your feedback.
> 
> Regards
> Naba
> 
> 
> On Mon, Dec 16, 2019 at 9:47 AM Kirk Lund  wrote:
> 
>> Whenever I submit a PR with multiple commits that I intend to rebase
>> to
>> develop, 

Re: [DISCUSS] Proposal to require linear commit history on develop

[Dan Smith]

-1 to (1) and (2).

I think merge commits are appropriate in certain circumstances, so I don't
think we should make a blanket restriction. In fact I think our release
process involves some merges.

I think setting standards on what is reasonable to be an individual commit
will do a lot more to clean up our history than blocking merges. We'd
rather not see commits like "Spotless Apply" in the history, but if
reasonably separate and well written commits come in as part of a merge, I
think that's fine.

-Dan

On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao  wrote:

> +1
>
> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:
>
> > I’d like to advance this topic from an informal request/discussion to a
> > discussion of a concrete proposal.
> >
> > To recap, it sounds like there is general agreement that commit history
> on
> > develop should be linear (no merge commits), and that the biggest
> obstacle
> > to this is that the PR merge button in GitHub creates a merge commit by
> > default.
> >
> > I propose the following changes:
> > 1) Change GitHub settings to remove the ability to create a merge commit.
> > This will make Squash & Merge the default.
> >
> > 2) Change GitHub settings to require linear history on develop.  This
> > prevents merge commits via command-line (not recommended, but wiki still
> > has instructions for merging PRs this way).
> >
> > 3) Update the PR template to change the text "Is your initial
> contribution
> > a single, squashed commit” to “Is your initial contribution squashed for
> > ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
> > ‘fix’ commit)"
> >
> > For clarity, I am proposing no-change in the following areas:
> > i) Leave Rebase & Merge as an option for PRs that have been structured to
> > benefit from it (this can make it easier in a bisect to see whether the
> > refactoring or the “fix” broke something).
> > ii) Leave existing wording in the wiki as-is [stating that squashing is
> > preferred].
> >
> >
> > Please comment via this email thread.
> > -Owen
> >
> >
> >
> > > On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
> > >
> > > I think it's already resolved Udo ;)
> > >
> > > Here's the problem, if I fixup a dunit test by removing all uses of
> > "this."
> > > and I rename the dunit test, then git doesn't remember that the file
> is a
> > > rename -- it forever afterwards interprets it as a new file that I
> > created
> > > if I touch more than 50% of the lines (which "this." can easily do). If
> > we
> > > squash two commits: the rename and the cleanup of that dunit test --
> then
> > > we effectively lose the history of that file and it shows that I
> created
> > a
> > > new file.
> > >
> > > Also for the record, I've been working on Geode since the beginning
> and I
> > > was never made aware of this change in our process. I never voted on
> it.
> > > I'm not a big fan of changing various details in our process every
> single
> > > week. It's very easy to miss these discussions unless someone points it
> > out
> > > to me.
> > >
> > > On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
> > > wrote:
> > >
> > >> I'm not sure what this discussion is about... WE, as a community, have
> > >> agreed in common practices, in two place no less...
> > >>
> > >> 1) Quoting our PR template
> > >>
> > >>
> > >>  For all changes:
> > >>
> > >>  *
> > >>
> > >>Is there a JIRA ticket associated with this PR? Is it referenced in
> > >>the commit message?
> > >>
> > >>  *
> > >>
> > >>Has your PR been rebased against the latest commit within the
> target
> > >>branch (typically|develop|)?
> > >>
> > >>  *
> > >>
> > >>***Is your initial contribution a single, squashed commit?*
> > >>
> > >>  *
> > >>
> > >>Does|gradlew build|run cleanly?
> > >>
> > >>  *
> > >>
> > >>Have you written or updated unit tests to verify your changes?
> > >>
> > >>  *
> > >>
> > >>If adding new dependencies to the code, are these dependencies
> > >>licensed in a way that is compatible for inclusion underASF 2.0
> > >>?
> > >>
> > >> On our PR template we call out that the initial PR commit should be
> > >> squashed.
> > >>
> > >> 2)
> https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
> > >>  >
> > >> -- See "Accepting a PR Using the Command Line" - Point #3.
> > >>
> > >>
> > >> @Kirk, if each of your commits "stands alone", I commend you on the
> > >> diligence, but in reality, they should either then be stand alone PR's
> > >> or just extra work created for yourself.
> > >>
> > >> If we want to change the way we have agreed upon we
> submit/commit/merge
> > >> changes back into develop... Then this is another discussion thread,
> > >> until then, I think we should all remind ourselves on our agreed
> > >> contributions code of conduct.
> > >>
> > >> --Udo
> > >>
> > >> On 12/16/19 9:59 AM, Nabarun Nag 

Re: [DISCUSS] Proposal to require linear commit history on develop

[Jacob Barrett]

I’m in agreement with Dan. Changes to the infrastructure to flat out prevent 
things that should be self policing is annoying. This PR review lock we have 
had already cost us valuable time waiting for PR pipelines to pass that have no 
relevance to the commit, like CI work: I’d hat to see yet another process 
enforced that Kees us from getting work done when necessary.

-Jake


> On Dec 19, 2019, at 4:43 PM, Dan Smith  wrote:
> 
> -1 to (1) and (2).
> 
> I think merge commits are appropriate in certain circumstances, so I don't
> think we should make a blanket restriction. In fact I think our release
> process involves some merges.
> 
> I think setting standards on what is reasonable to be an individual commit
> will do a lot more to clean up our history than blocking merges. We'd
> rather not see commits like "Spotless Apply" in the history, but if
> reasonably separate and well written commits come in as part of a merge, I
> think that's fine.
> 
> -Dan
> 
>> On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao  wrote:
>> 
>> +1
>> 
>>> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:
>>> 
>>> I’d like to advance this topic from an informal request/discussion to a
>>> discussion of a concrete proposal.
>>> 
>>> To recap, it sounds like there is general agreement that commit history
>> on
>>> develop should be linear (no merge commits), and that the biggest
>> obstacle
>>> to this is that the PR merge button in GitHub creates a merge commit by
>>> default.
>>> 
>>> I propose the following changes:
>>> 1) Change GitHub settings to remove the ability to create a merge commit.
>>> This will make Squash & Merge the default.
>>> 
>>> 2) Change GitHub settings to require linear history on develop.  This
>>> prevents merge commits via command-line (not recommended, but wiki still
>>> has instructions for merging PRs this way).
>>> 
>>> 3) Update the PR template to change the text "Is your initial
>> contribution
>>> a single, squashed commit” to “Is your initial contribution squashed for
>>> ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
>>> ‘fix’ commit)"
>>> 
>>> For clarity, I am proposing no-change in the following areas:
>>> i) Leave Rebase & Merge as an option for PRs that have been structured to
>>> benefit from it (this can make it easier in a bisect to see whether the
>>> refactoring or the “fix” broke something).
>>> ii) Leave existing wording in the wiki as-is [stating that squashing is
>>> preferred].
>>> 
>>> 
>>> Please comment via this email thread.
>>> -Owen
>>> 
>>> 
>>> 
 On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
 
 I think it's already resolved Udo ;)
 
 Here's the problem, if I fixup a dunit test by removing all uses of
>>> "this."
 and I rename the dunit test, then git doesn't remember that the file
>> is a
 rename -- it forever afterwards interprets it as a new file that I
>>> created
 if I touch more than 50% of the lines (which "this." can easily do). If
>>> we
 squash two commits: the rename and the cleanup of that dunit test --
>> then
 we effectively lose the history of that file and it shows that I
>> created
>>> a
 new file.
 
 Also for the record, I've been working on Geode since the beginning
>> and I
 was never made aware of this change in our process. I never voted on
>> it.
 I'm not a big fan of changing various details in our process every
>> single
 week. It's very easy to miss these discussions unless someone points it
>>> out
 to me.
 
 On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
 wrote:
 
> I'm not sure what this discussion is about... WE, as a community, have
> agreed in common practices, in two place no less...
> 
> 1) Quoting our PR template
> 
> 
> For all changes:
> 
> *
> 
>   Is there a JIRA ticket associated with this PR? Is it referenced in
>   the commit message?
> 
> *
> 
>   Has your PR been rebased against the latest commit within the
>> target
>   branch (typically|develop|)?
> 
> *
> 
>   ***Is your initial contribution a single, squashed commit?*
> 
> *
> 
>   Does|gradlew build|run cleanly?
> 
> *
> 
>   Have you written or updated unit tests to verify your changes?
> 
> *
> 
>   If adding new dependencies to the code, are these dependencies
>   licensed in a way that is compatible for inclusion underASF 2.0
>   ?
> 
> On our PR template we call out that the initial PR commit should be
> squashed.
> 
> 2)
>> https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
> >> 
> -- See "Accepting a PR Using the Command Line" - Point #3.
> 
> 
> @Kirk, if each of your commits "stands alone", I commend you on the
> diligence, but in rea

Re: [DISCUSS] Proposal to require linear commit history on develop

[Owen Nichols]

Dan, to address some of your concerns:

This proposal is to block merge commit on develop only.  The release process 
makes a merge commit to master only.

This proposal explicitly allows reasonably separate and well written commits in 
a single PR.  For this case simply choose Rebase & Merge, and all of the 
commits in the PR will be brought to develop individually (no merge commit 
wrapping them).

If you still feel strongly that merge commits should be allowed on develop, 
would you consider approving 1) to prevent accidental GitHub accidents, but 
veto only 2) to allow merge commits to be explicitly created manually when 
explicitly desired? 

> On Dec 19, 2019, at 4:43 PM, Dan Smith  wrote:
> 
> -1 to (1) and (2).
> 
> I think merge commits are appropriate in certain circumstances, so I don't
> think we should make a blanket restriction. In fact I think our release
> process involves some merges.
> 
> I think setting standards on what is reasonable to be an individual commit
> will do a lot more to clean up our history than blocking merges. We'd
> rather not see commits like "Spotless Apply" in the history, but if
> reasonably separate and well written commits come in as part of a merge, I
> think that's fine.
> 
> -Dan
> 
> On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao  wrote:
> 
>> +1
>> 
>> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:
>> 
>>> I’d like to advance this topic from an informal request/discussion to a
>>> discussion of a concrete proposal.
>>> 
>>> To recap, it sounds like there is general agreement that commit history
>> on
>>> develop should be linear (no merge commits), and that the biggest
>> obstacle
>>> to this is that the PR merge button in GitHub creates a merge commit by
>>> default.
>>> 
>>> I propose the following changes:
>>> 1) Change GitHub settings to remove the ability to create a merge commit.
>>> This will make Squash & Merge the default.
>>> 
>>> 2) Change GitHub settings to require linear history on develop.  This
>>> prevents merge commits via command-line (not recommended, but wiki still
>>> has instructions for merging PRs this way).
>>> 
>>> 3) Update the PR template to change the text "Is your initial
>> contribution
>>> a single, squashed commit” to “Is your initial contribution squashed for
>>> ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
>>> ‘fix’ commit)"
>>> 
>>> For clarity, I am proposing no-change in the following areas:
>>> i) Leave Rebase & Merge as an option for PRs that have been structured to
>>> benefit from it (this can make it easier in a bisect to see whether the
>>> refactoring or the “fix” broke something).
>>> ii) Leave existing wording in the wiki as-is [stating that squashing is
>>> preferred].
>>> 
>>> 
>>> Please comment via this email thread.
>>> -Owen
>>> 
>>> 
>>> 
 On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
 
 I think it's already resolved Udo ;)
 
 Here's the problem, if I fixup a dunit test by removing all uses of
>>> "this."
 and I rename the dunit test, then git doesn't remember that the file
>> is a
 rename -- it forever afterwards interprets it as a new file that I
>>> created
 if I touch more than 50% of the lines (which "this." can easily do). If
>>> we
 squash two commits: the rename and the cleanup of that dunit test --
>> then
 we effectively lose the history of that file and it shows that I
>> created
>>> a
 new file.
 
 Also for the record, I've been working on Geode since the beginning
>> and I
 was never made aware of this change in our process. I never voted on
>> it.
 I'm not a big fan of changing various details in our process every
>> single
 week. It's very easy to miss these discussions unless someone points it
>>> out
 to me.
 
 On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
 wrote:
 
> I'm not sure what this discussion is about... WE, as a community, have
> agreed in common practices, in two place no less...
> 
> 1) Quoting our PR template
> 
> 
> For all changes:
> 
> *
> 
>   Is there a JIRA ticket associated with this PR? Is it referenced in
>   the commit message?
> 
> *
> 
>   Has your PR been rebased against the latest commit within the
>> target
>   branch (typically|develop|)?
> 
> *
> 
>   ***Is your initial contribution a single, squashed commit?*
> 
> *
> 
>   Does|gradlew build|run cleanly?
> 
> *
> 
>   Have you written or updated unit tests to verify your changes?
> 
> *
> 
>   If adding new dependencies to the code, are these dependencies
>   licensed in a way that is compatible for inclusion underASF 2.0
>   ?
> 
> On our PR template we call out that the initial PR commit should be
> squashed.
> 
> 2)
>> https://cwiki.apache.org/confluence/display/GEODE/Code+contri

Re: [DISCUSS] Proposal to require linear commit history on develop

[Owen Nichols]

This proposal in no way prevents you from getting work done.  Squash is always 
enabled and always the most acceptable way to bring changes to develop.

Please start a separate thread if you would like to revisit the community 
decision to require passing PR checks.

> On Dec 19, 2019, at 4:49 PM, Jacob Barrett  wrote:
> 
> I’m in agreement with Dan. Changes to the infrastructure to flat out prevent 
> things that should be self policing is annoying. This PR review lock we have 
> had already cost us valuable time waiting for PR pipelines to pass that have 
> no relevance to the commit, like CI work: I’d hat to see yet another process 
> enforced that Kees us from getting work done when necessary.
> 
> -Jake
> 
> 
>> On Dec 19, 2019, at 4:43 PM, Dan Smith  wrote:
>> 
>> -1 to (1) and (2).
>> 
>> I think merge commits are appropriate in certain circumstances, so I don't
>> think we should make a blanket restriction. In fact I think our release
>> process involves some merges.
>> 
>> I think setting standards on what is reasonable to be an individual commit
>> will do a lot more to clean up our history than blocking merges. We'd
>> rather not see commits like "Spotless Apply" in the history, but if
>> reasonably separate and well written commits come in as part of a merge, I
>> think that's fine.
>> 
>> -Dan
>> 
>>> On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao  wrote:
>>> 
>>> +1
>>> 
 On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:
 
 I’d like to advance this topic from an informal request/discussion to a
 discussion of a concrete proposal.
 
 To recap, it sounds like there is general agreement that commit history
>>> on
 develop should be linear (no merge commits), and that the biggest
>>> obstacle
 to this is that the PR merge button in GitHub creates a merge commit by
 default.
 
 I propose the following changes:
 1) Change GitHub settings to remove the ability to create a merge commit.
 This will make Squash & Merge the default.
 
 2) Change GitHub settings to require linear history on develop.  This
 prevents merge commits via command-line (not recommended, but wiki still
 has instructions for merging PRs this way).
 
 3) Update the PR template to change the text "Is your initial
>>> contribution
 a single, squashed commit” to “Is your initial contribution squashed for
 ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
 ‘fix’ commit)"
 
 For clarity, I am proposing no-change in the following areas:
 i) Leave Rebase & Merge as an option for PRs that have been structured to
 benefit from it (this can make it easier in a bisect to see whether the
 refactoring or the “fix” broke something).
 ii) Leave existing wording in the wiki as-is [stating that squashing is
 preferred].
 
 
 Please comment via this email thread.
 -Owen
 
 
 
> On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
> 
> I think it's already resolved Udo ;)
> 
> Here's the problem, if I fixup a dunit test by removing all uses of
 "this."
> and I rename the dunit test, then git doesn't remember that the file
>>> is a
> rename -- it forever afterwards interprets it as a new file that I
 created
> if I touch more than 50% of the lines (which "this." can easily do). If
 we
> squash two commits: the rename and the cleanup of that dunit test --
>>> then
> we effectively lose the history of that file and it shows that I
>>> created
 a
> new file.
> 
> Also for the record, I've been working on Geode since the beginning
>>> and I
> was never made aware of this change in our process. I never voted on
>>> it.
> I'm not a big fan of changing various details in our process every
>>> single
> week. It's very easy to miss these discussions unless someone points it
 out
> to me.
> 
> On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
> wrote:
> 
>> I'm not sure what this discussion is about... WE, as a community, have
>> agreed in common practices, in two place no less...
>> 
>> 1) Quoting our PR template
>> 
>> 
>>For all changes:
>> 
>> *
>> 
>>  Is there a JIRA ticket associated with this PR? Is it referenced in
>>  the commit message?
>> 
>> *
>> 
>>  Has your PR been rebased against the latest commit within the
>>> target
>>  branch (typically|develop|)?
>> 
>> *
>> 
>>  ***Is your initial contribution a single, squashed commit?*
>> 
>> *
>> 
>>  Does|gradlew build|run cleanly?
>> 
>> *
>> 
>>  Have you written or updated unit tests to verify your changes?
>> 
>> *
>> 
>>  If adding new dependencies to the code, are these dependencies
>>  licensed in a way that is compatible for inclusion underASF 2.0
>>  

Re: [DISCUSS] Proposal to require linear commit history on develop

[Jacob Barrett]

If the appropriate or necessary process is a merge then this proposal prevents 
that. I am not interested in any hard restrictions like this. You proposal in 
response to dan is not palatable by disabling merges via github but allowing it 
manually. 

> On Dec 19, 2019, at 4:54 PM, Owen Nichols  wrote:
> 
> This proposal in no way prevents you from getting work done.  Squash is 
> always enabled and always the most acceptable way to bring changes to develop.
> 
> Please start a separate thread if you would like to revisit the community 
> decision to require passing PR checks.
> 
>> On Dec 19, 2019, at 4:49 PM, Jacob Barrett  wrote:
>> 
>> I’m in agreement with Dan. Changes to the infrastructure to flat out prevent 
>> things that should be self policing is annoying. This PR review lock we have 
>> had already cost us valuable time waiting for PR pipelines to pass that have 
>> no relevance to the commit, like CI work: I’d hat to see yet another process 
>> enforced that Kees us from getting work done when necessary.
>> 
>> -Jake
>> 
>> 
 On Dec 19, 2019, at 4:43 PM, Dan Smith  wrote:
>>> 
>>> -1 to (1) and (2).
>>> 
>>> I think merge commits are appropriate in certain circumstances, so I don't
>>> think we should make a blanket restriction. In fact I think our release
>>> process involves some merges.
>>> 
>>> I think setting standards on what is reasonable to be an individual commit
>>> will do a lot more to clean up our history than blocking merges. We'd
>>> rather not see commits like "Spotless Apply" in the history, but if
>>> reasonably separate and well written commits come in as part of a merge, I
>>> think that's fine.
>>> 
>>> -Dan
>>> 
 On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao  wrote:
 
 +1
 
> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols  wrote:
> 
> I’d like to advance this topic from an informal request/discussion to a
> discussion of a concrete proposal.
> 
> To recap, it sounds like there is general agreement that commit history
 on
> develop should be linear (no merge commits), and that the biggest
 obstacle
> to this is that the PR merge button in GitHub creates a merge commit by
> default.
> 
> I propose the following changes:
> 1) Change GitHub settings to remove the ability to create a merge commit.
> This will make Squash & Merge the default.
> 
> 2) Change GitHub settings to require linear history on develop.  This
> prevents merge commits via command-line (not recommended, but wiki still
> has instructions for merging PRs this way).
> 
> 3) Update the PR template to change the text "Is your initial
 contribution
> a single, squashed commit” to “Is your initial contribution squashed for
> ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
> ‘fix’ commit)"
> 
> For clarity, I am proposing no-change in the following areas:
> i) Leave Rebase & Merge as an option for PRs that have been structured to
> benefit from it (this can make it easier in a bisect to see whether the
> refactoring or the “fix” broke something).
> ii) Leave existing wording in the wiki as-is [stating that squashing is
> preferred].
> 
> 
> Please comment via this email thread.
> -Owen
> 
> 
> 
>> On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
>> 
>> I think it's already resolved Udo ;)
>> 
>> Here's the problem, if I fixup a dunit test by removing all uses of
> "this."
>> and I rename the dunit test, then git doesn't remember that the file
 is a
>> rename -- it forever afterwards interprets it as a new file that I
> created
>> if I touch more than 50% of the lines (which "this." can easily do). If
> we
>> squash two commits: the rename and the cleanup of that dunit test --
 then
>> we effectively lose the history of that file and it shows that I
 created
> a
>> new file.
>> 
>> Also for the record, I've been working on Geode since the beginning
 and I
>> was never made aware of this change in our process. I never voted on
 it.
>> I'm not a big fan of changing various details in our process every
 single
>> week. It's very easy to miss these discussions unless someone points it
> out
>> to me.
>> 
>> On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer 
>> wrote:
>> 
>>> I'm not sure what this discussion is about... WE, as a community, have
>>> agreed in common practices, in two place no less...
>>> 
>>> 1) Quoting our PR template
>>> 
>>> 
>>>   For all changes:
>>> 
>>> *
>>> 
>>> Is there a JIRA ticket associated with this PR? Is it referenced in
>>> the commit message?
>>> 
>>> *
>>> 
>>> Has your PR been rebased against the latest commit within the
 target
>>> branch (typically|develop|)?
>>> 
>>> *
>>> 
>>> **

Re: [DISCUSS] Proposal to require linear commit history on develop

[Nabarun Nag]

Hi all,

This is not a blanket restriction.
My suggestion, like last time, lets try it out and this time we don't even
need Apache Infra to step in. Feels like it has been requested in multiple
INFRA JIRAs by so many Apache projects to enable only the squash merge
button and disable the others, that this setting has been moved to a simple
entry in .asf.yml file in root git repository

Again this is a setting only for the develop branch. And can be reversed
with a simple commit.

I agree too much policing is a bad thing, but I want to look at it as a
safeguard. I have spoken to multiple developers who have mentioned they
clicked the wrong button in a hurry or the page refreshed and the commit
got merged in a wrong way.

Regards
Naba

Reference :
https://cwiki.apache.org/confluence/display/INFRA/.asf.yaml+features+for+git+repositories
Merge buttons

Projects can enable/disable the "merge PR" button in the GitHub UI and
configure which actions should be allowed by adding the following
configuration (or derivatives thereof):
github:
  enabled_merge_buttons:
# enable squash button:
squash:  true
# enable merge button:
merge:   true
# disable rebase button:
rebase:  false


On Thu, Dec 19, 2019 at 4:54 PM Owen Nichols  wrote:

> This proposal in no way prevents you from getting work done.  Squash is
> always enabled and always the most acceptable way to bring changes to
> develop.
>
> Please start a separate thread if you would like to revisit the community
> decision to require passing PR checks.
>
> > On Dec 19, 2019, at 4:49 PM, Jacob Barrett  wrote:
> >
> > I’m in agreement with Dan. Changes to the infrastructure to flat out
> prevent things that should be self policing is annoying. This PR review
> lock we have had already cost us valuable time waiting for PR pipelines to
> pass that have no relevance to the commit, like CI work: I’d hat to see yet
> another process enforced that Kees us from getting work done when necessary.
> >
> > -Jake
> >
> >
> >> On Dec 19, 2019, at 4:43 PM, Dan Smith  wrote:
> >>
> >> -1 to (1) and (2).
> >>
> >> I think merge commits are appropriate in certain circumstances, so I
> don't
> >> think we should make a blanket restriction. In fact I think our release
> >> process involves some merges.
> >>
> >> I think setting standards on what is reasonable to be an individual
> commit
> >> will do a lot more to clean up our history than blocking merges. We'd
> >> rather not see commits like "Spotless Apply" in the history, but if
> >> reasonably separate and well written commits come in as part of a
> merge, I
> >> think that's fine.
> >>
> >> -Dan
> >>
> >>> On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao  wrote:
> >>>
> >>> +1
> >>>
>  On Thu, Dec 19, 2019, 4:05 PM Owen Nichols 
> wrote:
> 
>  I’d like to advance this topic from an informal request/discussion to
> a
>  discussion of a concrete proposal.
> 
>  To recap, it sounds like there is general agreement that commit
> history
> >>> on
>  develop should be linear (no merge commits), and that the biggest
> >>> obstacle
>  to this is that the PR merge button in GitHub creates a merge commit
> by
>  default.
> 
>  I propose the following changes:
>  1) Change GitHub settings to remove the ability to create a merge
> commit.
>  This will make Squash & Merge the default.
> 
>  2) Change GitHub settings to require linear history on develop.  This
>  prevents merge commits via command-line (not recommended, but wiki
> still
>  has instructions for merging PRs this way).
> 
>  3) Update the PR template to change the text "Is your initial
> >>> contribution
>  a single, squashed commit” to “Is your initial contribution squashed
> for
>  ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
>  ‘fix’ commit)"
> 
>  For clarity, I am proposing no-change in the following areas:
>  i) Leave Rebase & Merge as an option for PRs that have been
> structured to
>  benefit from it (this can make it easier in a bisect to see whether
> the
>  refactoring or the “fix” broke something).
>  ii) Leave existing wording in the wiki as-is [stating that squashing
> is
>  preferred].
> 
> 
>  Please comment via this email thread.
>  -Owen
> 
> 
> 
> > On Dec 16, 2019, at 10:49 AM, Kirk Lund  wrote:
> >
> > I think it's already resolved Udo ;)
> >
> > Here's the problem, if I fixup a dunit test by removing all uses of
>  "this."
> > and I rename the dunit test, then git doesn't remember that the file
> >>> is a
> > rename -- it forever afterwards interprets it as a new file that I
>  created
> > if I touch more than 50% of the lines (which "this." can easily do).
> If
>  we
> > squash two commits: the rename and the cleanup of that dunit test --
> >>> then
> > we effectively lose the history of that file and it shows that I
> >>>

Re: [DISCUSS] `status locator` command fail when locator's ssl is turned on

[Jens Deppe]

So it seems that the situation is something like this where I'm able to
make the initial connection but then retrieving status fails:

gfsh>connect --security-properties-file=../security.properties

gfsh>status locator --name=locator1
Server version response invalid: This could be the result of trying to
connect a non-SSL-enabled client to an SSL-enabled locator.


It would seem very weird if I have to provide additional connection params
to the 'status' command if I've already provided them as part of the
'connect'. Could we not stash the connection properties (in the Gfsh
instance object) for subsequent usage?

--Jens

On Wed, Dec 18, 2019 at 9:04 AM Jinmei Liao  wrote:

> "status locator" command is broken on ssl enabled locators ever since we
> fixed a bug that leaked the connection properties from one tcp socket
> connection to another. Before that it would just magically work if we have
> previously made a successfully tcp connection to that same locator, now we
> are either required to find a way to specify the ssl properties when
> running the `status locator` command or change what we want to report back
> to the user.
>
> Here is what's happening now when we issue the command:
>
>1. it needs to retrieve two sets of info from locator: general info like
>(pid, working dir, status, jvm args etc) and whether cluster
> configuration
>service is running or not.
>2. when locator’s ssl is on, the retrieval of the cluster configuration
>info will always fail since it’s using a tcp connection to get that info
>and we currently don’t have the ssl security properties to connect.
>3. when locator’s ssl is on, the retrieval of the general info will
>mostly succeed except when user is only providing a host and port,
> there we
>would also need the ssl security properties in order to create an ssl
>socket.
>
>
> So we wither need to add an option for user to specify a
> `--security-properties-file` option to include all the ssl connection
> properties or change the behavior not to report cluster config status or
> not allowing host/port combination. (I am not here long enough to
> understand what users would use this command for, so this is just a random
> suggestions).
>
> Comments/thoughts?
>
> --
> Cheers
>
> Jinmei
>

Re: [DISCUSS] `status locator` command fail when locator's ssl is turned on

[Jinmei Liao]

that's a solution too. but since "status locator" can point to any locator
that's running, it may not be the one that the current gfsh is connected to.

On Thu, Dec 19, 2019 at 6:31 PM Jens Deppe  wrote:

> So it seems that the situation is something like this where I'm able to
> make the initial connection but then retrieving status fails:
>
> gfsh>connect --security-properties-file=../security.properties
>
> gfsh>status locator --name=locator1
> Server version response invalid: This could be the result of trying to
> connect a non-SSL-enabled client to an SSL-enabled locator.
>
>
> It would seem very weird if I have to provide additional connection params
> to the 'status' command if I've already provided them as part of the
> 'connect'. Could we not stash the connection properties (in the Gfsh
> instance object) for subsequent usage?
>
> --Jens
>
> On Wed, Dec 18, 2019 at 9:04 AM Jinmei Liao  wrote:
>
> > "status locator" command is broken on ssl enabled locators ever since we
> > fixed a bug that leaked the connection properties from one tcp socket
> > connection to another. Before that it would just magically work if we
> have
> > previously made a successfully tcp connection to that same locator, now
> we
> > are either required to find a way to specify the ssl properties when
> > running the `status locator` command or change what we want to report
> back
> > to the user.
> >
> > Here is what's happening now when we issue the command:
> >
> >1. it needs to retrieve two sets of info from locator: general info
> like
> >(pid, working dir, status, jvm args etc) and whether cluster
> > configuration
> >service is running or not.
> >2. when locator’s ssl is on, the retrieval of the cluster
> configuration
> >info will always fail since it’s using a tcp connection to get that
> info
> >and we currently don’t have the ssl security properties to connect.
> >3. when locator’s ssl is on, the retrieval of the general info will
> >mostly succeed except when user is only providing a host and port,
> > there we
> >would also need the ssl security properties in order to create an ssl
> >socket.
> >
> >
> > So we wither need to add an option for user to specify a
> > `--security-properties-file` option to include all the ssl connection
> > properties or change the behavior not to report cluster config status or
> > not allowing host/port combination. (I am not here long enough to
> > understand what users would use this command for, so this is just a
> random
> > suggestions).
> >
> > Comments/thoughts?
> >
> > --
> > Cheers
> >
> > Jinmei
> >
>


-- 
Cheers

Jinmei