Re: Geode 1.8 release pipeline

[Owen Nichols]

Pipeline is up: 
https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-release-1-8-0-main
 


> On Nov 12, 2018, at 3:09 PM, Alexander Murmann  wrote:
> 
> Hi everyone,
> 
> Would someone be able to set up a Concourse pipeline for the upcoming 1.8
> release?
> 
> Thank you very much!

Re: Spotless Check failing on Windows?

[Michael Oleske]

I'll look to make time to play with it and open a PR/Jira if I get anywhere
useful. I only bothered with this thread since it is jarring if you are a
new dev and clone the project and the build fails on first try.

-michael

On Monday, November 12, 2018, Patrick Rhomberg  wrote:

> Spotless has only been checking our gradle for maybe a week or two.  It
> enforces dependency configurations using parentheses and single-quotes so
> they would be easier to sort while some of us made dependencies explicit.
> (Also, a future spotless improvement, once I can shake the bugs out).
>
> The issue you're hitting is exactly what the lineEndings='unix' is supposed
> to alleviate.  We specify lineEndings='unix', although looking at the
> documentation now, DiffPlug "highly recommend[s] that you do not change
> this value" from its default of GIT_ATTRIBUTES.  (This has been the case
> since shortly after its introduction in October of 2016.)
>
> You could try changing the lineEndings value to GIT_ATTRIBUTES and update
> our .gitattributes to define a less-narrowly-defined eol.  (It currently
> appears to only target *.java files.)  I don't have a Windows environment
> up and running to test this on, but if you wanted to poke at these corners,
> it's likely where the issue lives.  Otherwise, we could coerce a platform
> check into spotlessCheck.upToDateWhen so that it is skipped on Windows, but
> I'd be morally opposed to that.
>
> Imagination is Change.
> ~Patrick
>
> [1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
> [2] https://help.github.com/articles/dealing-with-line-endings/
>
> On Sun, Nov 11, 2018 at 8:47 PM, Owen Nichols  wrote:
>
> > The pipeline does actually build on Windows, but with -x spotlessCheck
> >
> > Someone (maybe whoever’s touched th spotless rules most recently) might
> > know what to do. I tend to agree that the same source code ought to be
> able
> > to pass spotless on both Linux and windows; and our pipeline should
> > probably be tweaked to ensure this gets tested.
> >
> > On Sun, Nov 11, 2018 at 8:35 PM Michael Oleske 
> wrote:
> >
> > > Hi Geode Dev Friends!
> > >
> > > I was building the latest off of develop (hash 75d7ed8a49d6) in
> > PowerShell
> > > on my Windows box and noticed during build that the spotless check
> > failed.
> > > It strangely fails on line endings (\r\n fun times) on what seems to be
> > all
> > > (or at least 34 of them) build.gradle files.  This seems odd to me
> since
> > I
> > > didn't realize that spotless check actually checked more than just java
> > > files.  I assume no pipeline picked this up since the pipelines would
> > build
> > > once and pass the jar around rather than build on Windows and Linux
> > (since
> > > JVM and all that).  I haven't tracked down when this started happening
> to
> > > me (though I can if that is of interest to people).
> > >
> > > If anyone has any thoughts I'd like to hear (even if they are just why
> > > Windows why!)
> > >
> > > -michael
> > >
> >
>

Re: Release branch for Apache Geode 1.8.0 has been cut

[Ryan McMahon]

Hi Alexander,

I would like to cherry pick the following commits from develop to
release/1.8.0:

https://github.com/apache/geode/commit/e9ea18e18c85b977b91192d4edbb9a4e18b2643e
*Reason*: This was a revert of a previous commit which addressed data
inconsistencies between WAN sites, but it was found the fix introduced
several other inconsistencies.

https://github.com/apache/geode/commit/aab0198e8478d4246042b2eb889c8ce7e28bb52e
*Reason: *This fixes a race condition in the QueryMonitor which causes an
unexpected RejectedExecutionException in monitorQuery()

Please let me know if this sounds reasonable and I can go ahead and begin
the cherry picking process.

Thanks!
Ryan


On Thu, Nov 8, 2018 at 1:54 PM Alexander Murmann 
wrote:

> Hello everyone,
>
> As discussed previously created a new release branch for Apache Geode 1.8.0
> - "release/1.8.0"
>
> Please do review and raise any concern with the release branch.
> If no concerns are raised, we will start with the voting for the release
> candidate soon.
>
> This also means that all tickets JIRA that get resolved on develop should
> be marked with 1.9.0 as their fix version.
>
> Thanks!
>
> Alexander
>

A small proposal: Not Sorting in AnalyzeSerializablesJUnitTest

[Galen O'Sullivan]

Hi all,

I wrote a PR (GEODE-5800) recently to remove redundant cases from
DataSerializer.readObject etc. calls. This changed the bytecode size (but
not the behavior) of a number of DataSerializables, and I realized that the
task of updating the list (or viewing the diff) was made harder by the fact
that our sanctionedDataSerializables list has gotten out of order. I would
like to propose forcing the list (and probably sanctionedSerializables as
well) to be ordered and the files to be equal, as I see no benefit to
having the files out of order, and I do see a benefit to having
configuration files like this rigidly defined so that we can analyze and
read diffs better.

Thoughts?

Thanks,
Galen

Re: Is concourse down?

[Robert Houghton]

Does github say it merges cleanly? I thought their messaging was pretty
clear

On Nov 12, 2018 17:19, "Alexander Murmann"  wrote:

I wonder if we somehow could find a way to make the error message clearer.
Since it didn't merge cleanly, there is action I need to take as the
committer. However, I would never know that from the error message.

On Mon, Nov 12, 2018 at 4:34 PM Patrick Rhomberg 
wrote:


> See also the previous time this occurred.
>
> https://markmail.org/thread/apwupkbkg3qipygo
>
> On Mon, Nov 12, 2018 at 4:18 PM, Owen Nichols  wrote:
>
> > This error usually means it didn’t merge cleanly, not that concourse is
> > “down"
> >
> > > On Nov 12, 2018, at 3:58 PM, Kirk Lund  wrote:
> > >
> > > I'm trying to get my PR to run tests, but the latest concourse jobs
are
> > > failing with:
> > >
> > > rsync_code_down-OpenJDK8
> > > missing inputs: geode, instance-data
> > > archive-results-OpenJDK8
> > > missing inputs: concourse-metadata-resource, geode, geode-results
> > > delete_instance-OpenJDK8
> > > missing inputs: geode, instance-data
> > >
> > > Anyone know what that means?
> > >
> > > Thanks,
> > > Kirk
> >
> >
>

Re: Is concourse down?

[Alexander Murmann]

I wonder if we somehow could find a way to make the error message clearer.
Since it didn't merge cleanly, there is action I need to take as the
committer. However, I would never know that from the error message.

On Mon, Nov 12, 2018 at 4:34 PM Patrick Rhomberg 
wrote:

> See also the previous time this occurred.
>
> https://markmail.org/thread/apwupkbkg3qipygo
>
> On Mon, Nov 12, 2018 at 4:18 PM, Owen Nichols  wrote:
>
> > This error usually means it didn’t merge cleanly, not that concourse is
> > “down"
> >
> > > On Nov 12, 2018, at 3:58 PM, Kirk Lund  wrote:
> > >
> > > I'm trying to get my PR to run tests, but the latest concourse jobs are
> > > failing with:
> > >
> > > rsync_code_down-OpenJDK8
> > > missing inputs: geode, instance-data
> > > archive-results-OpenJDK8
> > > missing inputs: concourse-metadata-resource, geode, geode-results
> > > delete_instance-OpenJDK8
> > > missing inputs: geode, instance-data
> > >
> > > Anyone know what that means?
> > >
> > > Thanks,
> > > Kirk
> >
> >
>

Re: Is concourse down?

[Patrick Rhomberg]

See also the previous time this occurred.

https://markmail.org/thread/apwupkbkg3qipygo

On Mon, Nov 12, 2018 at 4:18 PM, Owen Nichols  wrote:

> This error usually means it didn’t merge cleanly, not that concourse is
> “down"
>
> > On Nov 12, 2018, at 3:58 PM, Kirk Lund  wrote:
> >
> > I'm trying to get my PR to run tests, but the latest concourse jobs are
> > failing with:
> >
> > rsync_code_down-OpenJDK8
> > missing inputs: geode, instance-data
> > archive-results-OpenJDK8
> > missing inputs: concourse-metadata-resource, geode, geode-results
> > delete_instance-OpenJDK8
> > missing inputs: geode, instance-data
> >
> > Anyone know what that means?
> >
> > Thanks,
> > Kirk
>
>

Re: Is concourse down?

[Owen Nichols]

This error usually means it didn’t merge cleanly, not that concourse is “down"

> On Nov 12, 2018, at 3:58 PM, Kirk Lund  wrote:
> 
> I'm trying to get my PR to run tests, but the latest concourse jobs are
> failing with:
> 
> rsync_code_down-OpenJDK8
> missing inputs: geode, instance-data
> archive-results-OpenJDK8
> missing inputs: concourse-metadata-resource, geode, geode-results
> delete_instance-OpenJDK8
> missing inputs: geode, instance-data
> 
> Anyone know what that means?
> 
> Thanks,
> Kirk

Is concourse down?

[Kirk Lund]

I'm trying to get my PR to run tests, but the latest concourse jobs are
failing with:

rsync_code_down-OpenJDK8
missing inputs: geode, instance-data
archive-results-OpenJDK8
missing inputs: concourse-metadata-resource, geode, geode-results
delete_instance-OpenJDK8
missing inputs: geode, instance-data

Anyone know what that means?

Thanks,
Kirk

Re: Geode 1.8 release pipeline

[Owen Nichols]

I will create it.

> On Nov 12, 2018, at 3:09 PM, Alexander Murmann  wrote:
> 
> Hi everyone,
> 
> Would someone be able to set up a Concourse pipeline for the upcoming 1.8
> release?
> 
> Thank you very much!

Geode 1.8 release pipeline

[Alexander Murmann]

Hi everyone,

Would someone be able to set up a Concourse pipeline for the upcoming 1.8
release?

Thank you very much!

Re: [DISCUSS] Disable merge for failing pull requests

[Jinmei Liao]

Just to clarify, that flaky EvictionDUnitTest is old flaky. The PR to
refactor the test passed all checks, even the repeatTest as well. I had a
closed PR that just touched the "un-refactored" EvictionDUnitTest, it
wouldn't even pass the repeatTest at all.

On Mon, Nov 12, 2018 at 2:04 PM Dan Smith  wrote:

> To be clear, I don't think we have an issue of untrustworthy committers
> pushing code they know is broken to develop.
>
> The problem is that it is all to easy to look at a PR with some failures
> and *assume* your code didn't cause the failures and merge it anyway. I
> think we should all be at least rerunning the tests and not merging the PR
> until everything passes. If the merge button is greyed out, that might help
> communicate that standard to everyone.
>
> Looking at the OpenJDK 8 metrics, it looks to me like most of the issues
> are recently introduced (builds 81-84 and the EvictionDUnitTest), not old
> flaky tests. So I think we were a little more disciplined always listening
> to what the checks are telling us we would have less noise in the long run.
>
>
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-metrics/jobs/GeodeDistributedTestOpenJDK8Metrics/builds/23
>
> -Dan
>
> On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer  wrote:
>
> > 0
> >
> > Patrick does make a point. The committers on the project have been voted
> > in as "responsible, trustworthy and best of breed" and rights and
> > privileges according to those beliefs have been bestowed.
> >
> > We should live those words and trust our committers. In the end.. If
> > there is a "rotten" apple in the mix this should be addressed, maybe not
> > as public flogging, but with stern communications.
> >
> > On the other side, I've also seen the model where the submitter of PR is
> > not allowed to merge + commit their own PR's. That befalls to another
> > committer to complete this task, avoiding the whole, "I'll just quickly
> > commit without due diligence".
> >
> > --Udo
> >
> >
> > On 11/12/18 10:23, Patrick Rhomberg wrote:
> > > -1
> > >
> > > I really don't think this needs to be codified.  If people are merging
> > red
> > > PRs, that is a failing as a responsible developer.  Defensive
> programming
> > > is all well and good, but this seems like it's a bit beyond the pale in
> > > that regard.  I foresee it making the correction of a red main pipeline
> > > must more difficult that it needs to be.  And while it's much better
> than
> > > it had been, I am (anecdotally) still seeing plenty of flakiness in my
> > PRs,
> > > either resulting from infra failures or test classes that need to be
> > > refactored or reevaluated.
> > >
> > > If someone is merging truly broken code that has failed precheckin,
> that
> > > seems to me to be a discussion to have with that person.   If it
> > > persists, perhaps a public flogging would be in order.  But at the
> > end
> > > of the day, the onus is on us to be responsible developers, and no
> amount
> > > of gatekeeping is going to be a substitute for that.
> > >
> > > On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan <
> gosulli...@pivotal.io
> > >
> > > wrote:
> > >
> > >> I'm in favor of this change, but only if we have a way to restart
> > failing
> > >> PR builds without being the PR submitter. Any committer should be able
> > to
> > >> restart the build. The pipeline is still flaky enough and I want to
> > avoid
> > >> the situation where a new contributor is asked repeatedly to push
> empty
> > >> commits to trigger a PR build (in between actual PR review) and their
> PR
> > >> gets delayed by days if not weeks. That's a real bad experience for
> the
> > >> people we want to be inviting in.
> > >>
> > >> On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann <
> amurm...@pivotal.io>
> > >> wrote:
> > >>
> > >>> What's the general consensus on flakiness of the pipeline for this
> > >> purpose?
> > >>> If there is consensus that it's still too flaky to disable the merge
> > >> button
> > >>> on failure, we should probably consider doubling down on that issue
> > >> again.
> > >>> It's hard to tell from just looking at the dev pipeline because you
> > >> cannot
> > >>> see easily what failures were legitimate.
> > >>>
> > >>> On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt <
> > bschucha...@pivotal.io
> > >>>
> > >>> wrote:
> > >>>
> >  I'm in favor of this.
> > 
> >  Several times over the years we've made a big push to get precheckin
> > to
> >  reliably only to see rapid degradation due to crappy code being
> pushed
> >  in the face of precheckin failures.  We've just invested another
> half
> >  year doing it again.  Are we going to do things differently now?
> >  Disabling the Merge button on test failure might be a good start.
> > 
> >  On 11/9/18 12:55 PM, Dan Smith wrote:
> > 
> > > Hi all,
> > >
> > > Kirks emails reminded me - I think we are at the point now with our
> > >> PR
> > > checks where we 

Re: [DISCUSS] Disable merge for failing pull requests

[Udo Kohlmeyer]

In that case, why don't we make the rule that the owner of the PR is not 
allowed to merge it.


Another committer must then accept and merge it.

--Udo


On 11/12/18 14:03, Dan Smith wrote:

To be clear, I don't think we have an issue of untrustworthy committers
pushing code they know is broken to develop.

The problem is that it is all to easy to look at a PR with some failures
and *assume* your code didn't cause the failures and merge it anyway. I
think we should all be at least rerunning the tests and not merging the PR
until everything passes. If the merge button is greyed out, that might help
communicate that standard to everyone.

Looking at the OpenJDK 8 metrics, it looks to me like most of the issues
are recently introduced (builds 81-84 and the EvictionDUnitTest), not old
flaky tests. So I think we were a little more disciplined always listening
to what the checks are telling us we would have less noise in the long run.

https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-metrics/jobs/GeodeDistributedTestOpenJDK8Metrics/builds/23

-Dan

On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer  wrote:


0

Patrick does make a point. The committers on the project have been voted
in as "responsible, trustworthy and best of breed" and rights and
privileges according to those beliefs have been bestowed.

We should live those words and trust our committers. In the end.. If
there is a "rotten" apple in the mix this should be addressed, maybe not
as public flogging, but with stern communications.

On the other side, I've also seen the model where the submitter of PR is
not allowed to merge + commit their own PR's. That befalls to another
committer to complete this task, avoiding the whole, "I'll just quickly
commit without due diligence".

--Udo


On 11/12/18 10:23, Patrick Rhomberg wrote:

-1

I really don't think this needs to be codified.  If people are merging

red

PRs, that is a failing as a responsible developer.  Defensive programming
is all well and good, but this seems like it's a bit beyond the pale in
that regard.  I foresee it making the correction of a red main pipeline
must more difficult that it needs to be.  And while it's much better than
it had been, I am (anecdotally) still seeing plenty of flakiness in my

PRs,

either resulting from infra failures or test classes that need to be
refactored or reevaluated.

If someone is merging truly broken code that has failed precheckin, that
seems to me to be a discussion to have with that person.   If it
persists, perhaps a public flogging would be in order.  But at the

end

of the day, the onus is on us to be responsible developers, and no amount
of gatekeeping is going to be a substitute for that.

On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan 
I'm in favor of this change, but only if we have a way to restart

failing

PR builds without being the PR submitter. Any committer should be able

to

restart the build. The pipeline is still flaky enough and I want to

avoid

the situation where a new contributor is asked repeatedly to push empty
commits to trigger a PR build (in between actual PR review) and their PR
gets delayed by days if not weeks. That's a real bad experience for the
people we want to be inviting in.

On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann 
wrote:


What's the general consensus on flakiness of the pipeline for this

purpose?

If there is consensus that it's still too flaky to disable the merge

button

on failure, we should probably consider doubling down on that issue

again.

It's hard to tell from just looking at the dev pipeline because you

cannot

see easily what failures were legitimate.

On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt <

bschucha...@pivotal.io

wrote:


I'm in favor of this.

Several times over the years we've made a big push to get precheckin

to

reliably only to see rapid degradation due to crappy code being pushed
in the face of precheckin failures.  We've just invested another half
year doing it again.  Are we going to do things differently now?
Disabling the Merge button on test failure might be a good start.

On 11/9/18 12:55 PM, Dan Smith wrote:


Hi all,

Kirks emails reminded me - I think we are at the point now with our

PR

checks where we should not be merging anything to develop that

doesn't

pass

all of the PR checks.

I propose we disable the merge button unless a PR is passing all of

the

checks. If we are in agreement I'll follow up with infra to see how

to

make

that happen.

This would not completely prevent pushing directly to develop from

the

command line, but since most developers seem to be using the github

UI, I

hope that it will steer people towards getting the PRs passing

instead

of

using the command line.

Thoughts?
-Dan

Re: [DISCUSS] Disable merge for failing pull requests

[Dan Smith]

To be clear, I don't think we have an issue of untrustworthy committers
pushing code they know is broken to develop.

The problem is that it is all to easy to look at a PR with some failures
and *assume* your code didn't cause the failures and merge it anyway. I
think we should all be at least rerunning the tests and not merging the PR
until everything passes. If the merge button is greyed out, that might help
communicate that standard to everyone.

Looking at the OpenJDK 8 metrics, it looks to me like most of the issues
are recently introduced (builds 81-84 and the EvictionDUnitTest), not old
flaky tests. So I think we were a little more disciplined always listening
to what the checks are telling us we would have less noise in the long run.

https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-metrics/jobs/GeodeDistributedTestOpenJDK8Metrics/builds/23

-Dan

On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer  wrote:

> 0
>
> Patrick does make a point. The committers on the project have been voted
> in as "responsible, trustworthy and best of breed" and rights and
> privileges according to those beliefs have been bestowed.
>
> We should live those words and trust our committers. In the end.. If
> there is a "rotten" apple in the mix this should be addressed, maybe not
> as public flogging, but with stern communications.
>
> On the other side, I've also seen the model where the submitter of PR is
> not allowed to merge + commit their own PR's. That befalls to another
> committer to complete this task, avoiding the whole, "I'll just quickly
> commit without due diligence".
>
> --Udo
>
>
> On 11/12/18 10:23, Patrick Rhomberg wrote:
> > -1
> >
> > I really don't think this needs to be codified.  If people are merging
> red
> > PRs, that is a failing as a responsible developer.  Defensive programming
> > is all well and good, but this seems like it's a bit beyond the pale in
> > that regard.  I foresee it making the correction of a red main pipeline
> > must more difficult that it needs to be.  And while it's much better than
> > it had been, I am (anecdotally) still seeing plenty of flakiness in my
> PRs,
> > either resulting from infra failures or test classes that need to be
> > refactored or reevaluated.
> >
> > If someone is merging truly broken code that has failed precheckin, that
> > seems to me to be a discussion to have with that person.   If it
> > persists, perhaps a public flogging would be in order.  But at the
> end
> > of the day, the onus is on us to be responsible developers, and no amount
> > of gatekeeping is going to be a substitute for that.
> >
> > On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan  >
> > wrote:
> >
> >> I'm in favor of this change, but only if we have a way to restart
> failing
> >> PR builds without being the PR submitter. Any committer should be able
> to
> >> restart the build. The pipeline is still flaky enough and I want to
> avoid
> >> the situation where a new contributor is asked repeatedly to push empty
> >> commits to trigger a PR build (in between actual PR review) and their PR
> >> gets delayed by days if not weeks. That's a real bad experience for the
> >> people we want to be inviting in.
> >>
> >> On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann 
> >> wrote:
> >>
> >>> What's the general consensus on flakiness of the pipeline for this
> >> purpose?
> >>> If there is consensus that it's still too flaky to disable the merge
> >> button
> >>> on failure, we should probably consider doubling down on that issue
> >> again.
> >>> It's hard to tell from just looking at the dev pipeline because you
> >> cannot
> >>> see easily what failures were legitimate.
> >>>
> >>> On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt <
> bschucha...@pivotal.io
> >>>
> >>> wrote:
> >>>
>  I'm in favor of this.
> 
>  Several times over the years we've made a big push to get precheckin
> to
>  reliably only to see rapid degradation due to crappy code being pushed
>  in the face of precheckin failures.  We've just invested another half
>  year doing it again.  Are we going to do things differently now?
>  Disabling the Merge button on test failure might be a good start.
> 
>  On 11/9/18 12:55 PM, Dan Smith wrote:
> 
> > Hi all,
> >
> > Kirks emails reminded me - I think we are at the point now with our
> >> PR
> > checks where we should not be merging anything to develop that
> >> doesn't
>  pass
> > all of the PR checks.
> >
> > I propose we disable the merge button unless a PR is passing all of
> >> the
> > checks. If we are in agreement I'll follow up with infra to see how
> >> to
>  make
> > that happen.
> >
> > This would not completely prevent pushing directly to develop from
> >> the
> > command line, but since most developers seem to be using the github
> >>> UI, I
> > hope that it will steer people towards getting the PRs passing
> >> instead
> >>> of
> > 

Re: [DISCUSS] Disable merge for failing pull requests

[Udo Kohlmeyer]

0

Patrick does make a point. The committers on the project have been voted 
in as "responsible, trustworthy and best of breed" and rights and 
privileges according to those beliefs have been bestowed.


We should live those words and trust our committers. In the end.. If 
there is a "rotten" apple in the mix this should be addressed, maybe not 
as public flogging, but with stern communications.


On the other side, I've also seen the model where the submitter of PR is 
not allowed to merge + commit their own PR's. That befalls to another 
committer to complete this task, avoiding the whole, "I'll just quickly 
commit without due diligence".


--Udo


On 11/12/18 10:23, Patrick Rhomberg wrote:

-1

I really don't think this needs to be codified.  If people are merging red
PRs, that is a failing as a responsible developer.  Defensive programming
is all well and good, but this seems like it's a bit beyond the pale in
that regard.  I foresee it making the correction of a red main pipeline
must more difficult that it needs to be.  And while it's much better than
it had been, I am (anecdotally) still seeing plenty of flakiness in my PRs,
either resulting from infra failures or test classes that need to be
refactored or reevaluated.

If someone is merging truly broken code that has failed precheckin, that
seems to me to be a discussion to have with that person.   If it
persists, perhaps a public flogging would be in order.  But at the end
of the day, the onus is on us to be responsible developers, and no amount
of gatekeeping is going to be a substitute for that.

On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan 
wrote:


I'm in favor of this change, but only if we have a way to restart failing
PR builds without being the PR submitter. Any committer should be able to
restart the build. The pipeline is still flaky enough and I want to avoid
the situation where a new contributor is asked repeatedly to push empty
commits to trigger a PR build (in between actual PR review) and their PR
gets delayed by days if not weeks. That's a real bad experience for the
people we want to be inviting in.

On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann 
wrote:


What's the general consensus on flakiness of the pipeline for this

purpose?

If there is consensus that it's still too flaky to disable the merge

button

on failure, we should probably consider doubling down on that issue

again.

It's hard to tell from just looking at the dev pipeline because you

cannot

see easily what failures were legitimate.

On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt 
I'm in favor of this.

Several times over the years we've made a big push to get precheckin to
reliably only to see rapid degradation due to crappy code being pushed
in the face of precheckin failures.  We've just invested another half
year doing it again.  Are we going to do things differently now?
Disabling the Merge button on test failure might be a good start.

On 11/9/18 12:55 PM, Dan Smith wrote:


Hi all,

Kirks emails reminded me - I think we are at the point now with our

PR

checks where we should not be merging anything to develop that

doesn't

pass

all of the PR checks.

I propose we disable the merge button unless a PR is passing all of

the

checks. If we are in agreement I'll follow up with infra to see how

to

make

that happen.

This would not completely prevent pushing directly to develop from

the

command line, but since most developers seem to be using the github

UI, I

hope that it will steer people towards getting the PRs passing

instead

of

using the command line.

Thoughts?
-Dan

Re: [DISCUSS] Disable merge for failing pull requests

[Patrick Rhomberg]

-1

I really don't think this needs to be codified.  If people are merging red
PRs, that is a failing as a responsible developer.  Defensive programming
is all well and good, but this seems like it's a bit beyond the pale in
that regard.  I foresee it making the correction of a red main pipeline
must more difficult that it needs to be.  And while it's much better than
it had been, I am (anecdotally) still seeing plenty of flakiness in my PRs,
either resulting from infra failures or test classes that need to be
refactored or reevaluated.

If someone is merging truly broken code that has failed precheckin, that
seems to me to be a discussion to have with that person.   If it
persists, perhaps a public flogging would be in order.  But at the end
of the day, the onus is on us to be responsible developers, and no amount
of gatekeeping is going to be a substitute for that.

On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan 
wrote:

> I'm in favor of this change, but only if we have a way to restart failing
> PR builds without being the PR submitter. Any committer should be able to
> restart the build. The pipeline is still flaky enough and I want to avoid
> the situation where a new contributor is asked repeatedly to push empty
> commits to trigger a PR build (in between actual PR review) and their PR
> gets delayed by days if not weeks. That's a real bad experience for the
> people we want to be inviting in.
>
> On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann 
> wrote:
>
> > What's the general consensus on flakiness of the pipeline for this
> purpose?
> > If there is consensus that it's still too flaky to disable the merge
> button
> > on failure, we should probably consider doubling down on that issue
> again.
> > It's hard to tell from just looking at the dev pipeline because you
> cannot
> > see easily what failures were legitimate.
> >
> > On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt  >
> > wrote:
> >
> > > I'm in favor of this.
> > >
> > > Several times over the years we've made a big push to get precheckin to
> > > reliably only to see rapid degradation due to crappy code being pushed
> > > in the face of precheckin failures.  We've just invested another half
> > > year doing it again.  Are we going to do things differently now?
> > > Disabling the Merge button on test failure might be a good start.
> > >
> > > On 11/9/18 12:55 PM, Dan Smith wrote:
> > >
> > > > Hi all,
> > > >
> > > > Kirks emails reminded me - I think we are at the point now with our
> PR
> > > > checks where we should not be merging anything to develop that
> doesn't
> > > pass
> > > > all of the PR checks.
> > > >
> > > > I propose we disable the merge button unless a PR is passing all of
> the
> > > > checks. If we are in agreement I'll follow up with infra to see how
> to
> > > make
> > > > that happen.
> > > >
> > > > This would not completely prevent pushing directly to develop from
> the
> > > > command line, but since most developers seem to be using the github
> > UI, I
> > > > hope that it will steer people towards getting the PRs passing
> instead
> > of
> > > > using the command line.
> > > >
> > > > Thoughts?
> > > > -Dan
> > > >
> > >
> > >
> >
>

Re: Spotless Check failing on Windows?

[Patrick Rhomberg]

Spotless has only been checking our gradle for maybe a week or two.  It
enforces dependency configurations using parentheses and single-quotes so
they would be easier to sort while some of us made dependencies explicit.
(Also, a future spotless improvement, once I can shake the bugs out).

The issue you're hitting is exactly what the lineEndings='unix' is supposed
to alleviate.  We specify lineEndings='unix', although looking at the
documentation now, DiffPlug "highly recommend[s] that you do not change
this value" from its default of GIT_ATTRIBUTES.  (This has been the case
since shortly after its introduction in October of 2016.)

You could try changing the lineEndings value to GIT_ATTRIBUTES and update
our .gitattributes to define a less-narrowly-defined eol.  (It currently
appears to only target *.java files.)  I don't have a Windows environment
up and running to test this on, but if you wanted to poke at these corners,
it's likely where the issue lives.  Otherwise, we could coerce a platform
check into spotlessCheck.upToDateWhen so that it is skipped on Windows, but
I'd be morally opposed to that.

Imagination is Change.
~Patrick

[1] https://github.com/diffplug/spotless/tree/master/plugin-gradle
[2] https://help.github.com/articles/dealing-with-line-endings/

On Sun, Nov 11, 2018 at 8:47 PM, Owen Nichols  wrote:

> The pipeline does actually build on Windows, but with -x spotlessCheck
>
> Someone (maybe whoever’s touched th spotless rules most recently) might
> know what to do. I tend to agree that the same source code ought to be able
> to pass spotless on both Linux and windows; and our pipeline should
> probably be tweaked to ensure this gets tested.
>
> On Sun, Nov 11, 2018 at 8:35 PM Michael Oleske  wrote:
>
> > Hi Geode Dev Friends!
> >
> > I was building the latest off of develop (hash 75d7ed8a49d6) in
> PowerShell
> > on my Windows box and noticed during build that the spotless check
> failed.
> > It strangely fails on line endings (\r\n fun times) on what seems to be
> all
> > (or at least 34 of them) build.gradle files.  This seems odd to me since
> I
> > didn't realize that spotless check actually checked more than just java
> > files.  I assume no pipeline picked this up since the pipelines would
> build
> > once and pass the jar around rather than build on Windows and Linux
> (since
> > JVM and all that).  I haven't tracked down when this started happening to
> > me (though I can if that is of interest to people).
> >
> > If anyone has any thoughts I'd like to hear (even if they are just why
> > Windows why!)
> >
> > -michael
> >
>

Re: [DISCUSS] Disable merge for failing pull requests

[Galen O'Sullivan]

I'm in favor of this change, but only if we have a way to restart failing
PR builds without being the PR submitter. Any committer should be able to
restart the build. The pipeline is still flaky enough and I want to avoid
the situation where a new contributor is asked repeatedly to push empty
commits to trigger a PR build (in between actual PR review) and their PR
gets delayed by days if not weeks. That's a real bad experience for the
people we want to be inviting in.

On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann 
wrote:

> What's the general consensus on flakiness of the pipeline for this purpose?
> If there is consensus that it's still too flaky to disable the merge button
> on failure, we should probably consider doubling down on that issue again.
> It's hard to tell from just looking at the dev pipeline because you cannot
> see easily what failures were legitimate.
>
> On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt 
> wrote:
>
> > I'm in favor of this.
> >
> > Several times over the years we've made a big push to get precheckin to
> > reliably only to see rapid degradation due to crappy code being pushed
> > in the face of precheckin failures.  We've just invested another half
> > year doing it again.  Are we going to do things differently now?
> > Disabling the Merge button on test failure might be a good start.
> >
> > On 11/9/18 12:55 PM, Dan Smith wrote:
> >
> > > Hi all,
> > >
> > > Kirks emails reminded me - I think we are at the point now with our PR
> > > checks where we should not be merging anything to develop that doesn't
> > pass
> > > all of the PR checks.
> > >
> > > I propose we disable the merge button unless a PR is passing all of the
> > > checks. If we are in agreement I'll follow up with infra to see how to
> > make
> > > that happen.
> > >
> > > This would not completely prevent pushing directly to develop from the
> > > command line, but since most developers seem to be using the github
> UI, I
> > > hope that it will steer people towards getting the PRs passing instead
> of
> > > using the command line.
> > >
> > > Thoughts?
> > > -Dan
> > >
> >
> >
>

Re: [DISCUSS] Disable merge for failing pull requests

[Alexander Murmann]

What's the general consensus on flakiness of the pipeline for this purpose?
If there is consensus that it's still too flaky to disable the merge button
on failure, we should probably consider doubling down on that issue again.
It's hard to tell from just looking at the dev pipeline because you cannot
see easily what failures were legitimate.

On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt 
wrote:

> I'm in favor of this.
>
> Several times over the years we've made a big push to get precheckin to
> reliably only to see rapid degradation due to crappy code being pushed
> in the face of precheckin failures.  We've just invested another half
> year doing it again.  Are we going to do things differently now?
> Disabling the Merge button on test failure might be a good start.
>
> On 11/9/18 12:55 PM, Dan Smith wrote:
>
> > Hi all,
> >
> > Kirks emails reminded me - I think we are at the point now with our PR
> > checks where we should not be merging anything to develop that doesn't
> pass
> > all of the PR checks.
> >
> > I propose we disable the merge button unless a PR is passing all of the
> > checks. If we are in agreement I'll follow up with infra to see how to
> make
> > that happen.
> >
> > This would not completely prevent pushing directly to develop from the
> > command line, but since most developers seem to be using the github UI, I
> > hope that it will steer people towards getting the PRs passing instead of
> > using the command line.
> >
> > Thoughts?
> > -Dan
> >
>
>

Re: [DISCUSS] Disable merge for failing pull requests

[Bruce Schuchardt]

I'm in favor of this.

Several times over the years we've made a big push to get precheckin to 
reliably only to see rapid degradation due to crappy code being pushed 
in the face of precheckin failures.  We've just invested another half 
year doing it again.  Are we going to do things differently now?  
Disabling the Merge button on test failure might be a good start.


On 11/9/18 12:55 PM, Dan Smith wrote:


Hi all,

Kirks emails reminded me - I think we are at the point now with our PR
checks where we should not be merging anything to develop that doesn't pass
all of the PR checks.

I propose we disable the merge button unless a PR is passing all of the
checks. If we are in agreement I'll follow up with infra to see how to make
that happen.

This would not completely prevent pushing directly to develop from the
command line, but since most developers seem to be using the github UI, I
hope that it will steer people towards getting the PRs passing instead of
using the command line.

Thoughts?
-Dan

Re: [DISCUSS] Deprecating JUnit4CacheTestCase (and maybe JUnit4DistributedTestCase?)

[Jinmei Liao]

I feel like we are mixing two topic in this discussion. One is to improve
our user API, and one is to write clean tests. Doing one doesn't have to
sacrifice the other. If our rules are using internal APIs, then it's a
chance to check if we can improve our APIs like Kirk has done. But stop
using the rules or change the rules to use a more verbose API should not
the way to go. My 2 cents.

On Mon, Nov 12, 2018 at 4:22 AM Juan José Ramos  wrote:

> Hello all,
>
> What about mixing both approaches?.
> We can use the *Rules* to avoid duplication of code *but re-write them* to
> directly use the Geode User APIs instead of the Geode Internal API.
> Just for the record, https://issues.apache.org/jira/browse/GEODE-5739 was
> created for something similar (use [Server|Locator]Launcher instead of
> internal API in [Server|Locator]StarterRule), but it didn't get enough
> consent to be merged (leaving aside the huge amount of unrelated changes in
> the PR, the idea itself was rejected).
> Cheers.
>
>
> On Fri, Nov 9, 2018 at 11:55 PM Jinmei Liao  wrote:
>
> > Yeah, I guess. But why going out of this way to avoid using rules? Why
> > rules are bad? Rules just encapsulate common befores and afters to avoid
> > duplicate code in every tests. I don't see why we should avoid using it.
> >
> > On Fri, Nov 9, 2018, 3:44 PM Galen O'Sullivan  > wrote:
> >
> > > I wonder if we could use some sort of assertions to validate that that
> > > tests have cleaned up, at least in a few ways? For example, if a
> > > Cache/Locator/DistributedSystem is still running after a test has
> > finished,
> > > that's a good sign of a dirty environment. If system properties are
> still
> > > set, that's a good sign of a dirty environment. We could use a custom
> > test
> > > runner, or even add a rule to all our tests en masse that checks that
> > > things are cleaned up.
> > >
> > > Jinmei, for single-JVM tests, you could write a method for your test
> (or
> > > test class) that sets whatever properties you need and returns a Cache
> > > constructed with those properties. Then you can use try-with-resources
> to
> > > make sure that the Cache is properly closed. Is that a good alternative
> > to
> > > using a rule?
> > >
> > > On Fri, Nov 9, 2018 at 3:28 PM Helena Bales  wrote:
> > >
> > > > +1 for deprecating old test bases. Many of the tests that gave me the
> > > most
> > > > trouble this summer were because of JUnit4CacheTestCase.
> > > > Also +1 for pulling out Blackboard into a rule.
> > > >
> > > > I will, however, argue for continuing to use ClusterStartupRule. The
> > > > benefit of that is that it makes sure that all JVMs started for
> servers
> > > and
> > > > locators are cleaned up at the end of the tests, even if the tests
> > fail.
> > > We
> > > > could certainly spend time making that code easier to understand,
> but I
> > > > don't think that starting clusters is straightforward enough to have
> > > > confidence that it will get done correctly every time. Multi-threaded
> > > tests
> > > > should be a cautionary tale for this; some implementations were fine,
> > but
> > > > many polluted the system with threads that never stopped and tests
> that
> > > > didn't actually test anything.
> > > > As I see it, we are paying in readability for tests that do things
> the
> > > > right way.
> > > >
> > > > On Fri, Nov 9, 2018 at 2:31 PM Kirk Lund  wrote:
> > > >
> > > > > I would love to see you apply some of your passion for that to
> > > improving
> > > > > the User APIs so there's less boiler plate code for the Users as
> > well.
> > > > >
> > > > > Please don't forget that to Developers who are not familiar with
> our
> > > > Rules
> > > > > such as ClusterStarterRule, that it can be very difficult to
> > understand
> > > > > what it has done and how it has configured Geode. The more the Rule
> > > does,
> > > > > the greater this problem. The fact that most of these Rules use
> > > Internal
> > > > > APIs instead of User APIs is also a problem in my opinion because
> > we're
> > > > not
> > > > > testing exactly what a User would do or can do.
> > > > >
> > > > > To many of us Developers, figuring out what all the rules have
> > > configured
> > > > > and done is a much bigger problem than it is to deal with verbose
> > code
> > > > in a
> > > > > setUp method that uses CacheFactory directly. On one hand I want to
> > > say,
> > > > do
> > > > > as you prefer but we also have to consider that other Developers
> need
> > > to
> > > > > maintain these tests that are using the Rules, so I will continue
> to
> > > > > advocate for the writing of tests using Geode User APIs as much as
> > > > possible
> > > > > for the reasons I already stated.
> > > > >
> > > > > On Fri, Nov 9, 2018 at 1:44 PM, Jinmei Liao 
> > wrote:
> > > > >
> > > > > > I like using the rules because it reduces boiler plate code and
> the
> > > > > chance
> > > > > > of not cleaning up properly after your tests. It also make what
> you
> > > are
> > > > > > really 

Re: [DISCUSS] Deprecating JUnit4CacheTestCase (and maybe JUnit4DistributedTestCase?)

[Juan José Ramos]

Hello all,

What about mixing both approaches?.
We can use the *Rules* to avoid duplication of code *but re-write them* to
directly use the Geode User APIs instead of the Geode Internal API.
Just for the record, https://issues.apache.org/jira/browse/GEODE-5739 was
created for something similar (use [Server|Locator]Launcher instead of
internal API in [Server|Locator]StarterRule), but it didn't get enough
consent to be merged (leaving aside the huge amount of unrelated changes in
the PR, the idea itself was rejected).
Cheers.


On Fri, Nov 9, 2018 at 11:55 PM Jinmei Liao  wrote:

> Yeah, I guess. But why going out of this way to avoid using rules? Why
> rules are bad? Rules just encapsulate common befores and afters to avoid
> duplicate code in every tests. I don't see why we should avoid using it.
>
> On Fri, Nov 9, 2018, 3:44 PM Galen O'Sullivan  wrote:
>
> > I wonder if we could use some sort of assertions to validate that that
> > tests have cleaned up, at least in a few ways? For example, if a
> > Cache/Locator/DistributedSystem is still running after a test has
> finished,
> > that's a good sign of a dirty environment. If system properties are still
> > set, that's a good sign of a dirty environment. We could use a custom
> test
> > runner, or even add a rule to all our tests en masse that checks that
> > things are cleaned up.
> >
> > Jinmei, for single-JVM tests, you could write a method for your test (or
> > test class) that sets whatever properties you need and returns a Cache
> > constructed with those properties. Then you can use try-with-resources to
> > make sure that the Cache is properly closed. Is that a good alternative
> to
> > using a rule?
> >
> > On Fri, Nov 9, 2018 at 3:28 PM Helena Bales  wrote:
> >
> > > +1 for deprecating old test bases. Many of the tests that gave me the
> > most
> > > trouble this summer were because of JUnit4CacheTestCase.
> > > Also +1 for pulling out Blackboard into a rule.
> > >
> > > I will, however, argue for continuing to use ClusterStartupRule. The
> > > benefit of that is that it makes sure that all JVMs started for servers
> > and
> > > locators are cleaned up at the end of the tests, even if the tests
> fail.
> > We
> > > could certainly spend time making that code easier to understand, but I
> > > don't think that starting clusters is straightforward enough to have
> > > confidence that it will get done correctly every time. Multi-threaded
> > tests
> > > should be a cautionary tale for this; some implementations were fine,
> but
> > > many polluted the system with threads that never stopped and tests that
> > > didn't actually test anything.
> > > As I see it, we are paying in readability for tests that do things the
> > > right way.
> > >
> > > On Fri, Nov 9, 2018 at 2:31 PM Kirk Lund  wrote:
> > >
> > > > I would love to see you apply some of your passion for that to
> > improving
> > > > the User APIs so there's less boiler plate code for the Users as
> well.
> > > >
> > > > Please don't forget that to Developers who are not familiar with our
> > > Rules
> > > > such as ClusterStarterRule, that it can be very difficult to
> understand
> > > > what it has done and how it has configured Geode. The more the Rule
> > does,
> > > > the greater this problem. The fact that most of these Rules use
> > Internal
> > > > APIs instead of User APIs is also a problem in my opinion because
> we're
> > > not
> > > > testing exactly what a User would do or can do.
> > > >
> > > > To many of us Developers, figuring out what all the rules have
> > configured
> > > > and done is a much bigger problem than it is to deal with verbose
> code
> > > in a
> > > > setUp method that uses CacheFactory directly. On one hand I want to
> > say,
> > > do
> > > > as you prefer but we also have to consider that other Developers need
> > to
> > > > maintain these tests that are using the Rules, so I will continue to
> > > > advocate for the writing of tests using Geode User APIs as much as
> > > possible
> > > > for the reasons I already stated.
> > > >
> > > > On Fri, Nov 9, 2018 at 1:44 PM, Jinmei Liao 
> wrote:
> > > >
> > > > > I like using the rules because it reduces boiler plate code and the
> > > > chance
> > > > > of not cleaning up properly after your tests. It also make what you
> > are
> > > > > really trying to do in the test stand out more in the test code.
> > > > >
> > > > > On Fri, Nov 9, 2018 at 1:37 PM Kirk Lund  wrote:
> > > > >
> > > > > > We need to pull out the DUnit Blackboard from DistributedTestCase
> > and
> > > > > > repackage it as a BlackboardRule. It makes sense to make that a
> > JUnit
> > > > > Rule
> > > > > > because it's not part of Geode or its User APIs but it's really
> > > useful
> > > > > for
> > > > > > distributed testing in general. It's also probably the last
> useful
> > > > thing
> > > > > > that's still in DistributedTestCase and no where else.
> > > > > >
> > > > > > On Fri, Nov 9, 2018 at 12:12 PM, Bruce Schuchardt <
> > > > >