Re: [PROPOSAL] annul support/1.15

[Dale Emery]

+1

From: Owen Nichols 
Sent: Wednesday, March 16, 2022 2:12 PM
To: geode 
Subject: [PROPOSAL] annul support/1.15

Seven weeks after cutting support/1.15, Jira now shows 11 blockers, up from 5 a 
few weeks ago.  I wonder if perhaps we cut the release branch prematurely?  I 
propose that we abandon this branch and focus on getting develop closer to what 
we want to ship, then discuss re-cutting the branch.

If this proposal is approved, I will archive support/1.15 as support/1.15.old, 
revert develop's numbering to 1.15.0, and bulk-update all Jira tickets fixed in 
1.16.0 to fixed in 1.15.0 instead.  Build numbering would start from 
1.15.0-build.1000 to easily distinguish pre- and post- recut.

Please vote/discuss with a goal of reaching consensus by 3PM PDT Monday Mar 21.

Thanks,
-Geode 1.15.0 Release Manager

Re: PR to add unique ID to DUnit log output

[Dale Emery]

Hi Jens and all,

A possibility to consider: Instead of generating an arbitrary-but-unique ID, 
use an existing identifier from the test environment… such as the ID in the 
test worker directory name. That might make it easier to map a given log line 
to the other artifacts from the test. So if the test worker dir was 
test-worker-98, then use 98 (or 98) as the ID in the logs. 
ProcessManager could get the ID by parsing the current working directory. Or we 
could change the “test isolation” stuff to set an environment variable that 
ProcessManager could use.

Another possibility to look into: Teach Gradle to distinguish different 
instances of the same test class, so that it doesn’t merge the logs. I don’t 
know whether this is possible, but it may be worth exploring. (The challenge is 
that Gradle assumes it will run a test class only once, and includes code to 
ensure that. We bypass that code in order to run a test class multiple times, 
but we don’t change how Gradle decides what log to append a test process’s 
output to.)

Dale

From: Jens Deppe 
Date: Monday, January 3, 2022 at 1:03 PM
To: dev@geode.apache.org 
Subject: PR to add unique ID to DUnit log output
Hi All.

Just a heads up that I have a PR up 
(https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F7232data=04%7C01%7Cdemery%40vmware.com%7C73f6134e922f4a69cbda08d9cefc883d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637768406280110715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=t5iFtadRhKMy3jHmplgrYt7TWdtVeVPDSHCUhBpSj2g%3Dreserved=0)
 which, if merged, will slightly change the log output from DUnit runs. The PR 
simply adds a 4 character unique ID to the log line. As in:

[vm0-51ec] [info 2021/12/24 15:43:54.367 UTC  ; tid=0x1d] Reinitializing JarDeploymentService with 
new working directory: null

[vm0-47b7] [info 2021/12/24 15:43:54.416 UTC   tid=0x1d] Reinitializing JarDeploymentService with 
new working directory: null

[vm1-47b7] [info 2021/12/24 15:43:54.431 UTC   tid=0x1d] Received method: 
org.apache.geode.test.dunit.internal.IdentifiableCallable.call with 0 args on 
object: IdentifiableCallable(0:start locator in vm0)

The ID is intended to be unique per test run when tests are run repeatedly.

I did this to assist in debugging test output from repeated test runs 
(StressNewTest) where individual tests’ log lines are simply interleaved making 
debugging very difficult.

The change, unfortunately, does not affect log lines generated from the test VM 
but only from VMs launched by the ProcessManager. If anyone has ideas how to do 
that, I’d love to hear them.

Well, actually one approach I’ve used is related to this PR 
(https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F7231data=04%7C01%7Cdemery%40vmware.com%7C73f6134e922f4a69cbda08d9cefc883d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637768406280110715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=EeSqSSXhmSnkW0548njAwUPquR9DkRjuZ7m4jcS%2B2c0%3Dreserved=0)
 which lets one name ExecutorServiceRule threads. Using this, I can use the 
ProcessManager.ID to name the executor threads and thus that ID becomes visible 
when the thread name is logged. Kinda hacky, but it’s still effective.

Anyway, this is not a call to review (although you’re welcome to do so) but 
just a FYI.

--Jens

Re: @TestOnly or @VisibleForTesting

[Dale Emery]

I’ve spent the last few hours analyzing uses of @VisibleForTesting. Within the 
first dozen or so I’ve discovered a few patterns.

@VisibleForTesting is a highly reliable indicator that:

  1.  The existence, name, and value/behavior of the annotated element are 
implementation details and not requirements. (No surprise here)
  2.  At least one test insists on these implementation details (even though 
they are not requirements).
  3.  The tests that use the annotated elements leave the actual responsibility 
untested. (The tests tend to treat the annotated elements as surrogates for the 
actual responsibility.)

That last one kinda scares me.

I also noted that some elements annotated as @VisibleForTesting are not 
referenced by any test.

Dale

From: Alexander Murmann 
Date: Thursday, November 4, 2021 at 5:02 PM
To: dev@geode.apache.org 
Subject: Re: @TestOnly or @VisibleForTesting
Another +1 to Eric's point. What are others seeing as valid use cases for those 
annotations?

From: Patrick Johnson 
Sent: Thursday, November 4, 2021 15:55
To: dev@geode.apache.org 
Subject: Re: @TestOnly or @VisibleForTesting

I agree with Eric. Maybe rather than standardizing on one, we should stop 
adding anymore @VisibleForTesting or @TestOnly to the codebase. Possibly 
deprecate @VisibleForTesting.

> On Nov 4, 2021, at 3:30 PM, Eric Zoerner  wrote:
>
> My opinion is that @VisibleForTesting is a code smell and either indicates 
> that there is refactoring needed or there are tests that are unnecessary. If 
> there is functionality in a private method that needs to be tested 
> independently, then that code should be extracted and placed in a separate 
> class that has a wider visibility that can be tested on its own.
>
> The same could probably be said for @TestOnly unless there are actually 
> static analysis tools that need it for some reason.
>
> From: Kirk Lund 
> Date: Thursday, November 4, 2021 at 15:13
> To: dev@geode.apache.org 
> Subject: Re: @TestOnly or @VisibleForTesting
> As everyone thinks about how we want to use these annotations, please keep
> this in mind that both *@VisibleForTesting* and *@TestOnly* can be used on
> Types (Class/Interface/Enum), Constructors, Methods and Fields. (not just
> Methods)
>
> On Thu, Nov 4, 2021 at 3:09 PM Kirk Lund  wrote:
>
>> Hey all,
>>
>> We're introducing a mess to the codebase. It's a small problem, but
>> several small problems become a big problem and one of my missions is to
>> clean up and improve the codebase.
>>
>> I recently started seeing lots of pull requests with usage of @TestOnly.
>> Sometimes it's used instead of @VisibleForTesting, while sometimes I see
>> both annotations added to the same method.
>>
>> Before we start using @TestOnly, I think we need some guidelines for when
>> to use @TestOnly versus @VisibleForTesting. Or if we're going to replace
>> @VisibleForTesting with @TestOnly, then we either need a PR for the
>> replacement or, at a minimum, deprecation annotation and javadocs added to
>> VisibleForTesting.java.
>>
>> The annotations appear similar but the javadocs describe slightly
>> different meanings for them...
>>
>> *@VisibleForTesting* was created in Geode several years ago to mean that
>> the method is either only for testing or the visibility of it was widened
>> (example: a private method might be widened to be package-private,
>> protected or public). The method might be used by product code, but it also
>> has widened scope specifically to allow tests to call it. The javadocs say:
>>
>> "Annotates a program element that exists, or is more widely visible than
>> otherwise necessary, only for use in test code.
>>
>> Introduced while mobbing with Michael Feathers. Name and javadoc borrowed
>> from Guava and AssertJ (both are Apache License 2.0)."
>>
>> *@TestOnly* started appearing when we added org.jetbrains.annotations
>> dependency earlier this year. It seems to indicate a method that is ONLY
>> used for tests (never called by product). The javadocs say:
>>
>> "A member or type annotated with TestOnly claims that it should be used
>> from testing code only.
>>
>> Apart from documentation purposes this annotation is intended to be used
>> by static analysis tools to validate against element contract violations.
>>
>> This annotation means that the annotated element exposes internal data and
>> breaks encapsulation of the containing class; the annotation won't prevent
>> its use from production code, developers even won't see warnings if their
>> IDE doesn't support the annotation. It's better to provide proper API which
>> can be used in production as well as in tests."
>>
>> So... when do we use one over the other? I don't think both annotations
>> should be on the same method. Also, some sort of guidelines are needed if
>> we're going to start using @TestOnly.
>>

Re: @TestOnly or @VisibleForTesting

[Dale Emery]

Kirk and I often use @VisibleForTesting as part of a technique for making 
complex code more testable.

Many classes have “hidden” dependencies that make that class hard to test. A 
useful technique is to add a constructor parameter to allow a test to inject a 
more controllable, more observable instance.

But we don’t want to force every use of the existing constructor to have to 
create that instance. So we leave the old constructor’s signature as is, create 
a new constructor that accepts that new parameter, and make the old constructor 
call the new one.

This new, intermediate constructor is called only by the old constructor and by 
tests. In order for tests to call it, we have to make it visible, so we mark it 
as @VisibleForTesting.

Dale


From: Alexander Murmann 
Date: Thursday, November 4, 2021 at 5:02 PM
To: dev@geode.apache.org 
Subject: Re: @TestOnly or @VisibleForTesting
Another +1 to Eric's point. What are others seeing as valid use cases for those 
annotations?

From: Patrick Johnson 
Sent: Thursday, November 4, 2021 15:55
To: dev@geode.apache.org 
Subject: Re: @TestOnly or @VisibleForTesting

I agree with Eric. Maybe rather than standardizing on one, we should stop 
adding anymore @VisibleForTesting or @TestOnly to the codebase. Possibly 
deprecate @VisibleForTesting.

> On Nov 4, 2021, at 3:30 PM, Eric Zoerner  wrote:
>
> My opinion is that @VisibleForTesting is a code smell and either indicates 
> that there is refactoring needed or there are tests that are unnecessary. If 
> there is functionality in a private method that needs to be tested 
> independently, then that code should be extracted and placed in a separate 
> class that has a wider visibility that can be tested on its own.
>
> The same could probably be said for @TestOnly unless there are actually 
> static analysis tools that need it for some reason.
>
> From: Kirk Lund 
> Date: Thursday, November 4, 2021 at 15:13
> To: dev@geode.apache.org 
> Subject: Re: @TestOnly or @VisibleForTesting
> As everyone thinks about how we want to use these annotations, please keep
> this in mind that both *@VisibleForTesting* and *@TestOnly* can be used on
> Types (Class/Interface/Enum), Constructors, Methods and Fields. (not just
> Methods)
>
> On Thu, Nov 4, 2021 at 3:09 PM Kirk Lund  wrote:
>
>> Hey all,
>>
>> We're introducing a mess to the codebase. It's a small problem, but
>> several small problems become a big problem and one of my missions is to
>> clean up and improve the codebase.
>>
>> I recently started seeing lots of pull requests with usage of @TestOnly.
>> Sometimes it's used instead of @VisibleForTesting, while sometimes I see
>> both annotations added to the same method.
>>
>> Before we start using @TestOnly, I think we need some guidelines for when
>> to use @TestOnly versus @VisibleForTesting. Or if we're going to replace
>> @VisibleForTesting with @TestOnly, then we either need a PR for the
>> replacement or, at a minimum, deprecation annotation and javadocs added to
>> VisibleForTesting.java.
>>
>> The annotations appear similar but the javadocs describe slightly
>> different meanings for them...
>>
>> *@VisibleForTesting* was created in Geode several years ago to mean that
>> the method is either only for testing or the visibility of it was widened
>> (example: a private method might be widened to be package-private,
>> protected or public). The method might be used by product code, but it also
>> has widened scope specifically to allow tests to call it. The javadocs say:
>>
>> "Annotates a program element that exists, or is more widely visible than
>> otherwise necessary, only for use in test code.
>>
>> Introduced while mobbing with Michael Feathers. Name and javadoc borrowed
>> from Guava and AssertJ (both are Apache License 2.0)."
>>
>> *@TestOnly* started appearing when we added org.jetbrains.annotations
>> dependency earlier this year. It seems to indicate a method that is ONLY
>> used for tests (never called by product). The javadocs say:
>>
>> "A member or type annotated with TestOnly claims that it should be used
>> from testing code only.
>>
>> Apart from documentation purposes this annotation is intended to be used
>> by static analysis tools to validate against element contract violations.
>>
>> This annotation means that the annotated element exposes internal data and
>> breaks encapsulation of the containing class; the annotation won't prevent
>> its use from production code, developers even won't see warnings if their
>> IDE doesn't support the annotation. It's better to provide proper API which
>> can be used in production as well as in tests."
>>
>> So... when do we use one over the other? I don't think both annotations
>> should be on the same method. Also, some sort of guidelines are needed if
>> we're going to start using @TestOnly.
>>

Using Ports Safely in Tests

[Dale Emery]

As of July, 2021, Geode's build system no longer executes test worker processes 
in separate Docker containers.

This increases the risk of port collisions between tests. Each test worker JVM 
and each Java process started by a test executes directly in the environment 
provided by the host machine. If a test is using a port, any concurrently 
running test that tries to bind to the same port number will suffer a 
BindException.
Safely allocating ports. To reduce this risk, Geode's build system now assigns 
each test worker JVM a distinct range of ports. In CI, which runs 24 
distributed tests concurrently, each test gets a range of about 400 ports.
In JVMs running Geode version 1.14 or later, AvailablePort and 
AvailablePortHelper will choose ports from this assigned range. As a result, no 
two tests will ever receive the same port number from these classes, no matter 
how many are running concurrently.
When you write a test, keep the following guidelines in mind, to make sure your 
test uses only the ranges of ports that the Geode build system assigned to it:

  *   To assign an available port, call AvailablePortHelper or AvailablePort. 
These are the only classes that know about the reduced port range for the test.
  *   Call AvailablePort and AvailablePortHelper only in the current version of 
Geode. Do not call these classes in Child VMs running older versions of Geode. 
Older versions of Geode do not honor the new, reduced port ranges.
  *   Do not launch any service using Geode’s default port for that service. 
Always explicitly assign an available port. The only safe use of default ports 
is in a test to verify that a service uses its default port by default. Note 
that such a test will fail the stress test CI job. To exclude such a test from 
the stress test job, annotate it with @Category(IgnoreInRepeatTestTasks.class).
  *   Do not launch any service using a port number hard-coded into the test. 
Always explicitly assign an available port. There is no safe use of any 
hard-coded port number in tests that can run concurrently.
  *   Do not attempt to reuse an ephemeral port. Some tests start a member on 
an ephemeral port, stop the member, and attempt to restart it on the same port. 
This is not safe. It was never safe. During the time between when the member 
stops and when it restarts, the port is available for the operating system to 
give to any other process. In CI, it is very, very likely that one or more 
concurrently-running tests (or other processes) will request ephemeral ports 
during the time when your test is not bound to it. If one of those processes 
gets the port your test was using, your test will fail.
This information is available on the Geode wiki: 
https://cwiki.apache.org/confluence/display/GEODE/Using+Ports+Safely+in+Tests

Dale

Re: Test failures on Windows with insufficient memory for the JRE while running distributed tests

[Dale Emery]

> *Do the Gfsh distributed tests on Windows leave behind more artifacts on
> the harddrive than other test targets?*

On Linux, the artifact file for a full distributed test run is ~750mb.

On Windows, the artifact file for just the gfsh distributed tests is ~1gb.

> *Are we running the Gfsh distributed tests in parallel (which might
> exacerbate harddrive swapping or memory consumption)?*

On Linux, the full distributed test suite executes as many as 24 test classes 
in parallel (each in its own test JVM).

On Windows, the gfsh distributed tests do not currently execute in parallel.

I don’t know the answers to the other questions.

Dale

From: Alberto Gomez 
Date: Wednesday, October 27, 2021 at 10:21 AM
To: dev@geode.apache.org 
Subject: Re: Test failures on Windows with insufficient memory for the JRE 
while running distributed tests
Thanks, Kirk.

Any expert on the OS images and pipeline could jump in to answer Kirk's 
questions and help?

Thanks,

Alberto

From: Kirk Lund 
Sent: Tuesday, October 26, 2021 7:26 PM
To: dev@geode.apache.org 
Subject: Re: Test failures on Windows with insufficient memory for the JRE 
while running distributed tests

PS: I should also mention that the *windows-gfsh-distributed* test target
is only run on Windows (never on Linux). It might be useful to try getting
windows-gfsh-distributed running on LInux to see if it hits the same issue
on that OS. This would also require some help from a pipeline expert.

On Tue, Oct 26, 2021 at 10:22 AM Kirk Lund  wrote:

> Hi Alberto,
>
> 32 kb is a very small amount of memory, so I don't think it's related to
> Java Heap. Based on what little I've read today, I think a failure in
> ChunkPool::allocate is probably related to either *running out of swap
> space or running out of address space in a 32 bit JVM*. Since the
> failures are OS specific, I would suspect the machine image we use for
> Windows to be involved.
>
> I also notice that this ChunkPool::allocate failure is only occurring for
> the Gfsh distributed tests which is the only job run on Windows that uses
> Gradle support for *JUnit Categories*. The Gradle target is
> distributedTest which we have configured with "*forkEvery 1*" which
> causes every test class to launch in a new JVM. Gradle implements JUnit
> 4 Category filtering by launching every test class to check the Categories
> and then either executes the tests or terminates without running any
> depending on the Categories.
>
> Some things I would check (or ask others about):
>
> *Is the harddrive space much smaller than what's available to the JVM(s)
> on Linux?*
>
> *Do the Gfsh distributed tests on Windows leave behind more artifacts on
> the harddrive than other test targets?*
>
> *Is it possible that the tests are using a 32-bit JVM on Windows? Or maybe
> the tests are spawning Gfsh process(es) using a 32-bit JVM instead of
> 64-bit?*
>
> *Are we running the Gfsh distributed tests in parallel (which might
> exacerbate harddrive swapping or memory consumption)?*
>
> Unfortunately, I don't know what most of the options in
> jinja.variables.yml are about. I think it would be best to get help from an
> expert in the OS images and pipeline details.
>
> Cheers,
> Kirk
>
> On Tue, Oct 26, 2021 at 12:59 AM Alberto Gomez 
> wrote:
>
>> Hi,
>>
>> I am having issues with insufficient memory for the Java Runtime
>> Environment when running some tests on the CI under Windows from the
>> following PR :
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F7006data=04%7C01%7Cdemery%40vmware.com%7C7b81184e5afb47b705f808d9996e46eb%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637709521186740352%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=ut262y%2FKb9hEjnEBC9UmRyx6CUPCvrsbDF7q%2B13NQMg%3Dreserved=0
>>
>> The tests never fail under Linux.
>>
>> This is the error I get for some VMs:
>>
>> [vm4] # There is insufficient memory for the Java Runtime Environment to
>> continue.
>> [vm4] # Native memory allocation (malloc) failed to allocate 32744 bytes
>> for ChunkPool::allocate
>>
>> I have reduced the amount of resources used originally by the tests but
>> still I am not able to get a clean execution.
>>
>> I do not know if it is a matter of changing the parameters for the
>> windows execution in ci/pipelines/shared/jinja.variables.yml or if there is
>> anything else to consider.
>>
>> I would appreciate if someone from the community could help me
>> troubleshoot this issue.
>>
>> Thanks in advance,
>>
>> Alberto
>>
>>
>>

Added Support for JUnit 5

[Dale Emery]

In September 2021, Geode added support for writing and running tests using 
JUnit 5.

Most Geode modules now support JUnit 5. For most Geode modules, you can now 
write each test class using either JUnit 5's "Jupiter" API or the legacy JUnit 
4 API.

Which modules support JUnit 5? Any source set that depends on geode-junit or 
geode-dunit already has JUnit 5 support. For those source sets you can start 
writing tests using the JUnit Jupiter API now, and Gradle will run them.

To add JUnit 5 support to a module or source set: Add lines like these to the 
"dependencies" configuration of the module’s build.gradle file:


testImplementation('org.junit.jupiter:junit-jupiter-api')

testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine')

The first line allows you to write unit tests using the JUnit Jupiter API. The 
second line allows Gradle to run your JUnit Jupiter unit tests.

To use JUnit Jupiter to write and run other kinds of tests (e.g. 
integrationTest or distributedTest), add similar lines to configure the 
appropriate source sets.

LIMITATIONS

  *   Because Geode support for JUnit Jupiter is so new, we have not yet added 
test framework code that takes advantage of its features.
  *   JUnit Jupiter does not support the use of Rules.

SEE ALSO

  *   The JUnit 5 User Guide: https://junit.org/junit5/docs/current/user-guide/
  *   Using JUnit 5 (a copy of this message on the Geode wiki): 
https://cwiki.apache.org/confluence/display/GEODE/Using+JUnit+5

Dale Emery

Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs

[Dale Emery]

Count me as -0. I have some concerns, but I’m okay trying this and seeing how 
it goes.

Dale

From: Owen Nichols 
Date: Wednesday, June 9, 2021 at 2:25 PM
To: dev@geode.apache.org 
Subject: Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs
Summarizing this thread so far:

In favor of making stress-new non-required:
Kirk
Mark
Myself

In favor of making all PR checks non-required:
Jake

In favor of hashing out a more nuanced balance between making it possible (but 
not too easy) to ignore stress-new failures:
Donal
Dale

Maybe that's rough concensus in terms of which option to recommend, but hardly 
a thread I would feel comfortable citing as evidence that the community is on 
board with striking down a longstanding protection that was enacted through a 
[VOTE].

On 6/9/21, 2:11 PM, "Anthony Baker"  wrote:

If we have consensus, no need to VOTE.

> On Jun 9, 2021, at 12:52 PM, Owen Nichols  wrote:
>
> Ok, I'm on board with changing stress-new-test from a required PR check 
to non-required.  It's simple, codeowners still have the final say, and it 
neatly avoids the whole topic of overrides.  Time to take a [VOTE]?
>

Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs

[Dale Emery]

Here’s the kind of scenario I’m imagining:

  *   Code owners and other reviewers review the PR in the usual way (either 
before or after the tests finish).
  *   Stress new test fails, perhaps multiple times.
  *   The committer investigates and, upon concluding that the failed tests are 
not related to the change, requests an override from the code owners.
  *   Code owners review the failures and the committer’s justification, and 
decide whether to override the failures.

In this scenario, the extra burden on code owners arises only at the 
committer’s request.

Dale

From: Owen Nichols 
Date: Wednesday, June 9, 2021 at 11:15 AM
To: dev@geode.apache.org 
Subject: Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs
This would substantially increase the burden on codeowners, because now in 
addition to looking at the code itself, they would have to wait for any running 
PR checks to complete, as well as remember to come back and look at the PR 
after any subsequent commits to make sure the checks are still passing.

On 6/9/21, 10:56 AM, "Mark Hanson"  wrote:

I think that process is a bit much. PRs are already a challenge. What do 
people think about code owners being the gate. We can accept by custom that 
there should be no stress-new-test failures. If there is a failure, a code 
owner can request a change or decide to let it go. I think that is sufficient 
all things considered.

Thanks,
Mark

On 6/9/21, 10:43 AM, "Owen Nichols"  wrote:

I feel that a traditional [DISCUSS] and [VOTE] on the dev list would be 
sufficient and proper to grant approval for an override.  Any PR already needs 
approval from 1 codeowner per area to merge, so codeowners already have a 
little more say because they hold veto power over the PR.

In terms of "practicalities of how this would actually work":
Step 1: start a [DISCUSS] thread explaining the problem and why you 
think an override is justified
Step 2: if there is concensus, [VOTE]
Step 3: Myself (or whoever performs the override) must cite a link to 
the vote thread


    On 6/9/21, 10:16 AM, "Dale Emery"  wrote:

I too like #1 best for now… assuming it’s possible to give code 
owners this ability.

Coincidentally, about option #3, II was reading the git release 
notes just now, and noticed there’s a new “trailers” feature. It gives git the 
ability to parse “key: value” pairs at the end of a commit message. We could 
potentially (with a sufficiently current version of git) use that to exclude a 
test from a PR stress test run.

And, yeah, option #2 brings back the @FlakyTest annotation that we 
worked so hard to eliminate.

As Mark said, none of this fixes the underlying problem, which I’d 
frame as: We have too many tests whose results we don’t trust.

Dale

From: Kirk Lund 
Date: Wednesday, June 9, 2021 at 9:59 AM
To: dev@geode.apache.org 
Subject: Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement 
from PRs
I do like the suggestions offered up by Dale and would encourage 
(or even
plead) with my fellow contributors to consider these:

  *   Allow code owners to override the block, if they can be 
convinced the
> override is justified.
>   *   Exclude troublesome tests from stress test runs, either via
> annotations or via an `assumeThat(…)` that can detect that it’s 
running as
> a stress test. Whatever the mechanism for excluding, it would be 
in the
> code, and therefore subject to code owner review. (This, too, 
feels overly
> broad to me, as it would exclude the test from all stress test 
runs.)
>   *   A way to exclude a specific test method from running in the 
stress
> tests for a specific PR or commit. I don’t have any ideas for how 
to
> declare such an exclusion, but if it could be done via a file it 
would be
> subject to code owner review.


1) Allow code owners to override the block, if they can be 
convinced the
override is justified.

After all, if we don't trust our code owners...

2-3) Use a custom annotation to exclude the test method or test 
class only
from stress-new-test.

At first I really liked this idea, but then we end up with growing a
collection of flaky tests that are excluded in some way from
stress-new-test that still occasionally fail in distributedTest.

#1 really sounds like the best option to me. I believe that leaving 
our
stress-new-test process as-is will only discourage everyone from 
fixing one
or two flaky tests in a large dunit test. However, I also believe 
that if
w

Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs

[Dale Emery]

I too like #1 best for now… assuming it’s possible to give code owners this 
ability.

Coincidentally, about option #3, II was reading the git release notes just now, 
and noticed there’s a new “trailers” feature. It gives git the ability to parse 
“key: value” pairs at the end of a commit message. We could potentially (with a 
sufficiently current version of git) use that to exclude a test from a PR 
stress test run.

And, yeah, option #2 brings back the @FlakyTest annotation that we worked so 
hard to eliminate.

As Mark said, none of this fixes the underlying problem, which I’d frame as: We 
have too many tests whose results we don’t trust.

Dale

From: Kirk Lund 
Date: Wednesday, June 9, 2021 at 9:59 AM
To: dev@geode.apache.org 
Subject: Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs
I do like the suggestions offered up by Dale and would encourage (or even
plead) with my fellow contributors to consider these:

  *   Allow code owners to override the block, if they can be convinced the
> override is justified.
>   *   Exclude troublesome tests from stress test runs, either via
> annotations or via an `assumeThat(…)` that can detect that it’s running as
> a stress test. Whatever the mechanism for excluding, it would be in the
> code, and therefore subject to code owner review. (This, too, feels overly
> broad to me, as it would exclude the test from all stress test runs.)
>   *   A way to exclude a specific test method from running in the stress
> tests for a specific PR or commit. I don’t have any ideas for how to
> declare such an exclusion, but if it could be done via a file it would be
> subject to code owner review.


1) Allow code owners to override the block, if they can be convinced the
override is justified.

After all, if we don't trust our code owners...

2-3) Use a custom annotation to exclude the test method or test class only
from stress-new-test.

At first I really liked this idea, but then we end up with growing a
collection of flaky tests that are excluded in some way from
stress-new-test that still occasionally fail in distributedTest.

#1 really sounds like the best option to me. I believe that leaving our
stress-new-test process as-is will only discourage everyone from fixing one
or two flaky tests in a large dunit test. However, I also believe that if
we give code owners the authority to override stress-new-test, then we
need to encourage them not to override this too often.

On Tue, Jun 8, 2021 at 11:11 AM Dale Emery  wrote:

> Maybe we can find a way to relax the requirement, or to allow addressing
> specific situations like the tangle you find yourself in.
>
> Removing the requirement altogether feels overly broad. I fear it would
> allow us to quietly disregard all intermittent test failures, and I think
> we already quietly (or even actively) disregard way too many kinds of
> failures.
>
> I would prefer some way to explicitly disregard only the specific test
> failures that prevent us from merging, and only with some amount of
> explicit justification.
>
> I’m not sure what that would look like. Some half-baked possibilities:
>
>   *   Allow code owners to override the block, if they can be convinced
> the override is justified.
>   *   Exclude troublesome tests from stress test runs, either via
> annotations or via an `assumeThat(…)` that can detect that it’s running as
> a stress test. Whatever the mechanism for excluding, it would be in the
> code, and therefore subject to code owner review. (This, too, feels overly
> broad to me, as it would exclude the test from all stress test runs.)
>   *   A way to exclude a specific test method from running in the stress
> tests for a specific PR or commit. I don’t have any ideas for how to
> declare such an exclusion, but if it could be done via a file it would be
> subject to code owner review.
>
> Dale
>
> From: Kirk Lund 
> Date: Tuesday, June 8, 2021 at 9:33 AM
> To: dev@geode.apache.org 
> Subject: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs
> Our requirement for stress-new-test-openjdk11 to pass before allowing merge
> doesn't really work as intended for fixing distributed tests that contain
> multiple flaky test methods. In fact, I think it causes contributors to
> avoid tackling flaky tests.
>
> I've been working on GEODE-9103: CI Failure:
> PutAllClientServerDistributedTest.testPutAllReturnsExceptions FAILED
> <https://issues.apache.org/jira/browse/GEODE-9103> and was able to fix it.
>
> However, stress-new-test-openjdk11 then continued to fail for other flaky
> tests in PutAllClientServerDistributedTest. I managed to fix GEODE-9296 and
> GEODE-8528 as well. I also tried but have not been able to fix GEODE-9242
> which remains flaky.
>
> Unfortunately, I cannot merge any of my fixes for
> PutAllClientSer

Re: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs

[Dale Emery]

Maybe we can find a way to relax the requirement, or to allow addressing 
specific situations like the tangle you find yourself in.

Removing the requirement altogether feels overly broad. I fear it would allow 
us to quietly disregard all intermittent test failures, and I think we already 
quietly (or even actively) disregard way too many kinds of failures.

I would prefer some way to explicitly disregard only the specific test failures 
that prevent us from merging, and only with some amount of explicit 
justification.

I’m not sure what that would look like. Some half-baked possibilities:

  *   Allow code owners to override the block, if they can be convinced the 
override is justified.
  *   Exclude troublesome tests from stress test runs, either via annotations 
or via an `assumeThat(…)` that can detect that it’s running as a stress test. 
Whatever the mechanism for excluding, it would be in the code, and therefore 
subject to code owner review. (This, too, feels overly broad to me, as it would 
exclude the test from all stress test runs.)
  *   A way to exclude a specific test method from running in the stress tests 
for a specific PR or commit. I don’t have any ideas for how to declare such an 
exclusion, but if it could be done via a file it would be subject to code owner 
review.

Dale

From: Kirk Lund 
Date: Tuesday, June 8, 2021 at 9:33 AM
To: dev@geode.apache.org 
Subject: [DISCUSS] Remove stress-new-test-openjdk11 requirement from PRs
Our requirement for stress-new-test-openjdk11 to pass before allowing merge
doesn't really work as intended for fixing distributed tests that contain
multiple flaky test methods. In fact, I think it causes contributors to
avoid tackling flaky tests.

I've been working on GEODE-9103: CI Failure:
PutAllClientServerDistributedTest.testPutAllReturnsExceptions FAILED
 and was able to fix it.

However, stress-new-test-openjdk11 then continued to fail for other flaky
tests in PutAllClientServerDistributedTest. I managed to fix GEODE-9296 and
GEODE-8528 as well. I also tried but have not been able to fix GEODE-9242
which remains flaky.

Unfortunately, I cannot merge any of my fixes for
PutAllClientServerDistributedTest unless every single flaky test in it is
fixed by my PR. I think this is undesirable because it would be better to
merge the fix for 3 flaky test methods than none.

UPDATE: After running my precheckin more times, I did get
stress-new-test-openjdk11 to pass once so I can merge, but that's more of a
loophole than anything because I didn't manage to fix GEODE-9242.

Despite having PR #6542 eventually pass, I would like to discuss removing
or relaxing our requirement that stress-new-test-openjdk11 must pass in
order to merge a PR...

PR #6542: GEODE-9103: Fix ServerConnectivityExceptions in
PutAllClientServerDistributedTest


Fixed in PR #6542:
* GEODE-9296: CI Failure: PutAllClientServerDistributedTest >
testPartialKeyInPRSingleHopWithRedundancy

* GEODE-9103: CI Failure:
PutAllClientServerDistributedTest.testPutAllReturnsExceptions FAILED

* GEODE-8528: PutAllClientServerDistributedTest.testPartialKeyInPRSingleHop
fails due to missing disk store after server restart


Still flaky:
* GEODE-9242: CI failure in PutAllClientServerDistributedTest >
testEventIdOutOfOrderInPartitionRegionSingleHop


Thanks,
Kirk

Re: [Discuss] New feature work approval state in Geode project?

[Dale Emery]

Does this mean that a Jira would have to be approved before assigned?

Dale

From: Mark Hanson 
Date: Friday, May 28, 2021 at 10:36 AM
To: dev@geode.apache.org 
Subject: [Discuss] New feature work approval state in Geode project?
Hi All,

There has been some discussion about adding a new state of approved to Geode 
Jira for features or something like it, to help prevent work being done that 
doesn’t make it into the project. What to people think?

Thanks,
Mark

Re: Cleaning up the codebase - use curly braces everywhere

[Dale Emery]

We might also use IntelliJ to enforce any guidelines that we want to enforce. 
You can run inspections on the command line: 
https://www.jetbrains.com/help/idea/command-line-code-inspector.html

An advantage of using IntelliJ inspections is that we can provide an inspection 
profile that treats violations as errors. Then developers can use this profile 
while editing to spot violations immediately as they’re introduced.

A disadvantage is that this somewhat couples Geode to a particular IDE.

Dale

From: Donal Evans 
Date: Thursday, May 27, 2021 at 10:22 AM
To: dev@geode.apache.org 
Subject: Cleaning up the codebase - use curly braces everywhere
Hi Geode dev,

I've recently been looking at ways to improve code quality in small ways 
throughout the codebase, and as a starting point, I thought it would be good to 
make it so that we're consistently using curly braces for control flow 
statements everywhere, since this is something that's specifically called out 
in the Geode Code Style Guide wiki page[1] as one of the "more important 
points" of our code style.

IntelliJ has a "Run inspection by name..." feature that makes it possible to 
identify all places where curly braces aren't used for control flow statements, 
(which showed over 3300 occurrences in the codebase) and also allows them to be 
automatically inserted, making the fix relatively trivial. Since this PR will 
touch 640 files, I wanted to make sure to first check that this is something 
even worth doing, and, if there's agreement that it is, to give reviewers 
context on what the changes are, the motivation for them, and how they were 
made, to help with the review process.

The draft PR I have up[2] currently has no failing tests and can be marked as 
ready to review if there aren't any objections, and once it is, I'll try to 
coordinate with codeowners to get the minimal number of approvals required for 
a merge (it looks like only 6-7 reviewers are needed, though I'm sure that 
almost every code owner will be tagged as reviewers given the number of files 
touched).

If this idea is a success, I think it would be good to have a discussion about 
other low-hanging code improvements we could make using static analysis 
(unnecessary casts, unused variables, duplicate conditions etc.), and, once a 
particular inspection has been "fixed," possibly consider adding a check for it 
as part of the PR pre-checkin to make sure it's not reintroduced. All thoughts 
and feedback are very welcome.

Donal

[1] https://cwiki.apache.org/confluence/display/GEODE/Code+Style+Guide
[2] 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523data=04%7C01%7Cdemery%40vmware.com%7Ca2cc7d62b84049fff7bc08d921340535%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637577329575017080%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=ZmyQo5GL6fqtQ7twsI1mDK%2BT0K6%2B6CAiO0Nba5ZkFAw%3Dreserved=0

Re: [VOTE] Requiring final keyword on every parameter and local variable

[Dale Emery]

My test for whether enforce a guideline in a PR review is: Would I be willing 
to automate enforcing it in CI?

I am -0 on this particular guideline.

IntelliJ offers two competing inspections for Java coding style:

  *   Local variable or parameter can be final
  *   Unnecessary 'final' on local variable or parameter

Both of these are (I think) disabled by default in IntelliJ. And neither 
satisfies my “I’d be willing to automate enforcing it” test.

Dale

From: Mark Hanson 
Date: Monday, April 19, 2021 at 11:08 AM
To: dev@geode.apache.org 
Subject: Re: [VOTE] Requiring final keyword on every parameter and local 
variable
Hi Jake,

I agree with everyone's point about final being a useful, I just don't find it 
useful enough to do anything manually across the code base at this point.

I believe first in foremost in no code warnings. Intellij warns about variables 
that can be final, so I make them final as it finds them. It is very rare that 
I am writing new code at this point. I have spent the last year bug fixing 
other people's code.  From my standpoint, original intent is largely moot. So, 
for me, the question is do I go through code that is not mine and mark it all 
as final (where appropriate)? Sure, in code that I touch. In code the rest of 
the codebase, I think there are bigger fish to fry before we get to final.

It seems like the larger portion of the consensus is to recommend that 
variables are marked final (where appropriate) as we find them or create them.
That seems like the going forward approach consensus.

I will be blunt though. This seems nitpicky *compared* to the number of code 
warnings there are and the fact that people are not actively fixing all of the 
warnings.
If we don't come to the same consensus about all warnings this seems like 
painting a rusted car, sure it makes it all look the same, but does very little 
for the underlying problems. Now to undermine my own argument a little, I think 
that especially in release blockers, we want to touch as little code as 
possible, so I am flexible there.

I would also like to agree with Udo about final really not being very good 
compared to Mutable and Immutable objects in other languages.

Thanks,
Mark


On 4/15/21, 7:49 PM, "Udo Kohlmeyer"  wrote:

@jake, you are correct, I did miss the LOCAL variable out of the subject. 
:face_palm:

Yes, adding "final" to LOCAL variables will be HUGELY beneficial in this 
regard. Have we seen any performance improvement after having refactored this 
method? Have you been able to measure any improvements?

--Udo

On 4/15/21, 1:19 PM, "Jacob Barrett"  wrote:



> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer  wrote:
> @Jake the idea of smaller methods is great and we should ALWAYS 
strive for that. But that argument is completely irrelevant in this discussion. 
As making method arguments final does not naturally guide a developer to 
creating smaller methods. Nor does a smaller method mean it can/will be jitted. 
Too many factors (to discuss here) are part of that decision, also it is not 
relevant in this discussion. But more on that topic read THIS.

The original subject is in regards to parameters and local variables.

Irrelevant is certainly an opinion you are welcome to have but let me 
challenge you. Goto DistributedCacheOperation._distribute().

First challenge, look at around line 333:
boolean reliableOp = isOperationReliable() && 
region.requiresReliabilityCheck();

Without scrolling do you see that variable used? Nope, because it is 
first used on line 439, ~100 lines away. Does it mutate between there, well I 
can search for all uses and find out or I could be nice to the next person and 
intend for it to never mutate by adding final. Intent communicated!

Second challenge, mark all the local variables in the method as final. 
Now make it compile without introducing more mutable variables. At the end of 
this journey you will have about a dozen unit testable methods and a 
_distribute method that goes from ~370 lines to  ~90 with no mutable local 
variables.

I argue it is relevant as good guardrail for writing good code. While 
we should ALWAYS strive for it we don’t. Every little nudge helps.


-Jake

Upcoming upgrade to Gradle 6.8.3

[Dale Emery]

On Monday I will merge GEODE-8899 (https://github.com/apache/geode/pull/6280), 
which upgrades the Geode build to use Gradle 6.8.3 (from the current Gradle 
5.5). This has implications for IntelliJ IDEA and for running tests with 
parallelDunit. The implications are minor, but you’ll probably be affected by 
at least the IntelliJ ones. See below for details.

IntelliJ IDEA. Once I merge this upgrade, IntelliJ IDEA may initially become 
confused. If that happens, you will likely need to do one or more of:

  *   Invalidate IntelliJ IDEA’s caches

 *   Select File > Invalidate Caches / Restart…
 *   Select Invalidate and Restart

  *   Re-import Geode into IntelliJ IDEA. See the instructions in the 
BUILDING.md file, which will be updated by this commit.

Running tests with parallelDunit. If you use -PparallelDunit to run tests in 
Docker, either in a script or on the command line, you will need to change a 
few command line options:

  *   -PdunitParallelForks= is no longer used. It has been replaced by 
--max-workers and testMaxParallelForks.
  *   --max-workers= specifies the maximum number of worker processes for 
Gradle to run in parallel. Set that to whatever value you used for 
dunitParallelForks.
  *   -PtestMaxParallelForks= specifies the maximum number of tests for each 
Dockerized test task to run in parallel. Set that to the value you used for 
dunitParallelForks.

Dale

Re: [Discussion] RFC to make Geode's working directory configurable

[Dale Emery]

Geode makes nearly 300 references to the JVM working directory. Tests make an 
additional 1200 references.

Implementing this proposal requires changing nearly all of these references so 
that, instead of resolving relative pathnames against the JVM's working 
directory, they resolve against the configured working directory.

There are several ways in which Geode and its tests reference the JVM's working 
directory:

1. Use of the "user.dir" system property.

2. Use of a File or Path with pathname "." or "".

3. Calls to methods that convert a possibly relative pathname to an absolute 
one:
- file.getAbsoluteFile()
- file.getAbsolutePath()
- file.getCanonicalFile()
- file.getCanonicalPath()
- path.toAbsolutePath()
- IOUtils.tryGetCanonicalFileElseGetAbsoluteFile(file)
- IOUtils.tryGetCanonicalPathElseGetAbsolutePath(file)

Changing these references involves varying degress of difficulty, depending on 
whether there is an object is in scope that can identify the configured working 
directory. Some references have ready access to a DistributionConfig, or to a 
cache from which it can retrieve the configuration. Many references are in 
contexts in which no configuration object is readily available.

In a small number of cases, it is appropriate to resolve relative pathnames 
against the JVM's working directory. These few references would not have to 
change.

See the attached PDF file for an inventory of how different modules reference 
the JVM's working directory.

Cheers,
Dale



On 10/6/20, 12:12 PM, "Dale Emery"  wrote:

Hi all,

I have submitted an RFC to make Geode’s working directory configurable: 
https://cwiki.apache.org/confluence/display/GEODE/Make+Geode%27s+Working+Directory+Configurable

Please review it and comment by Oct 26.

Cheers,
Dale

Re: [Discussion] RFC to make Geode's working directory configurable

[Dale Emery]

Hi Jake,

Production code will use it.

In my initial (very cursory) scan of current uses of user.dir, it looked as if 
some uses were in places that didn't have ready access to a cache or other good 
source for this property. It may have been hasty for me to leap to a singleton.

I will take a closer look at whether those uses have access to an object that 
can own the working dir, or if they can be given access to an owner object. If 
they do, I'll put the responsibility in an appropriate instance instead of a 
singleton.

Dale

On 10/6/20, 5:24 PM, "Jacob Barrett"  wrote:

Do we expect this to be used by production code or just test code? If this 
is going to be used by production code I am concerned with introducing another 
singleton class into the mix. We really want to be moving towards a 
non-singleton world where I can have more than one Cache in a JVM. For 
production code this value should probably be retrieved from the Cache, 
DistributedSystem or some child of those instances. If this is for test code 
only then ignore me the above concerns.

> On Oct 6, 2020, at 12:12 PM, Dale Emery  wrote:
> 
> Hi all,
> 
> I have submitted an RFC to make Geode’s working directory configurable: 
https://cwiki.apache.org/confluence/display/GEODE/Make+Geode%27s+Working+Directory+Configurable
> 
> Please review it and comment by Oct 26.
> 
> Cheers,
> Dale
> 

Re: [Discussion] RFC to make Geode's working directory configurable

[Dale Emery]

Hi Dan,

I spent more than a week scouring Gradle docs and code for any way to give the 
parallel forks their own working directories. I couldn't find a way. At least, 
not through the public API. And I'm reluctant to rely on internal APIs, the way 
our docker and repeat tests do. If this working-dir idea offers enough 
resistance, I'll take another look at Gradle.

Cheers,
Dale

On 10/6/20, 3:58 PM, "Dan Smith"  wrote:

+1

Looks good to me. If this is just for tests, I suspect there is some gradle 
way to make parallel forks use different working directories. But having this 
option in the product doesn't seem like a bad idea.

-Dan

From: Dale Emery 
Sent: Tuesday, October 6, 2020 12:12 PM
To: dev@geode.apache.org 
Subject: [Discussion] RFC to make Geode's working directory configurable

Hi all,

I have submitted an RFC to make Geode’s working directory configurable: 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FMake%2BGeode%2527s%2BWorking%2BDirectory%2BConfigurabledata=02%7C01%7Cdemery%40vmware.com%7C8d03aba161d24b80071108d86a4b5fe8%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637376219249446597sdata=wF6%2B%2BwsQX32AhndhvslF%2BeeKgA1e7YwVTMZmqoHED5o%3Dreserved=0

Please review it and comment by Oct 26.

Cheers,
Dale

[Discussion] RFC to make Geode's working directory configurable

[Dale Emery]

Hi all,

I have submitted an RFC to make Geode’s working directory configurable: 
https://cwiki.apache.org/confluence/display/GEODE/Make+Geode%27s+Working+Directory+Configurable

Please review it and comment by Oct 26.

Cheers,
Dale

Access to Geode GCP project

[Dale Emery]

I would like to use the Geode GCP project. How can I get access to that?

—
Dale Emery
dem...@vmware.com<mailto:dem...@vmware.com>

Re: Us vs Docker vs Gradle vs JUnit

[Dale Emery]

> On Jun 30, 2020, at 12:28 PM, Jinmei Liao  wrote:
> 
> I would vote for fixing the tests to use gradle's normal forking. If we are 
> going to invest time and effort, let's invest in an option that can reduce 
> our dependencies

I agree wholeheartedly!

Dale

Re: Docker on Windows

[Dale Emery]

Here are my notes from my most recent attempts:

• November
• Added JUnit 5 to geode-junit API
• Configured geode-junit and geode-core to run all tests via JUnit 5
• CI failures on JDK 11
• NPE thrown apparently by Gradle
• December
• Ran tests in JDK 11 on my Mac
• Failures do not occur on my Mac
• Set gradle’s log level to info
• Results in CI logs so enormous that Chrome cannot display them in a bearable 
timeframe
• Then the whole PR CI pipeline became unusable
• So I was able to gain no further information about the cause of the gradle 
NPEs

Here is the branch the way I left things: 
https://github.com/demery-pivotal/geode/tree/old/geode-junit5

About the PR CI pipeline becoming unusable… I don’t have any notes about what 
“unusable” means. I suspect it was unrelated to my PR, but I’m not sure.

Cheers,
Dale


On Jun 29, 2020, at 10:51 AM, Jacob Barrett 
mailto:jabarr...@vmware.com>> wrote:

Dale,

Sorry I thought it was Kirk. Do you have a branch somewhere with your work so 
far? Can you refresh us all on the issues you hit and what you think the next 
steps would be?

Thanks,
Jake


On Jun 29, 2020, at 10:23 AM, Kirk Lund 
mailto:kl...@apache.org>> wrote:

It was Dale who worked on migrating Geode to use JUnit 5. I know he ran
into some issues but I don't recall what they were. I'm definitely up for
helping on the JUnit 5 front!

On Fri, Jun 26, 2020 at 11:30 AM Jacob Barrett 
mailto:jabarr...@vmware.com>> wrote:

If the effort to do both is less than the sum of each individually then I
say lets do it.

Kirk, I recall you putting some effort into JUnit 5 at some point.

-Jake


On Jun 26, 2020, at 11:13 AM, Jens Deppe 
mailto:jde...@vmware.com>> wrote:

A bigger effort (but I think more correct and sustainable) would be to
switch to junit 5 where something like this could more easily be
implemented.

--Jens

On 6/26/20, 9:11 AM, "Robert Houghton" 
mailto:rhough...@vmware.com>> wrote:

 The plugin code that spawns junit test workers on containers needs
some serious help. Aside from the benefit we would get on windows, we also
are blocked on getting to the next major version of Gradle with the current
tool. I really think it might be easier to write our own Gradle plugin at
this point.


 From: Jacob Barrett mailto:jabarr...@vmware.com>>
 Date: Thursday, June 25, 2020 at 12:26 PM
 To: dev@geode.apache.org<mailto:dev@geode.apache.org> 
mailto:dev@geode.apache.org>>
 Subject: Docker on Windows

On Jun 25, 2020, at 12:23 PM, Jens Deppe 
mailto:jde...@vmware.com>> wrote:
It's been a couple of years since Sai and I tried (but failed) to
dockerize the tests. I'm sure docker support has improved and it might be
worth trying that again.

 Docker on windows has improved a lot but wasn’t the major issue the
docker plugin for Gradle needed some serious work?

 I have recently been experimenting with the Docker/Kubernetes for
Windows experience. Perhaps we can take another stab at this issue.

 -Jake





—
Dale Emery
dem...@vmware.com<mailto:dem...@vmware.com>

Re: Proposal to restore Pulse logging in support/1.12

[Dale Emery]

Merged into support/1.12. Thanks all.

> On Apr 24, 2020, at 3:25 PM, Mark Bretl  wrote:
> 
> +1
> 
> On Fri, Apr 24, 2020 at 3:07 PM Owen Nichols  wrote:
> 
>> +1
>> 
>>> On Apr 24, 2020, at 3:06 PM, Jinmei Liao  wrote:
>>> 
>>> +1
>>> 
>>> On Fri, Apr 24, 2020 at 2:53 PM Anthony Baker  wrote:
>>> 
>>>> +1
>>>> 
>>>>> On Apr 24, 2020, at 2:46 PM, Dale Emery  wrote:
>>>>> 
>>>>> During the cleanup of the gradle build and logging, the Pulse webapp
>>>> lost its slf4j implementation. As a result, Pulse stopped writing log
>> files.
>>>>> 
>>>>> I’ve restored Pulse logging in the develop branch.
>>>>> 
>>>>> I would like to restore it in support/1.12. I’ve created a PR:
>>>> https://github.com/apache/geode/pull/4992 <
>>>> https://github.com/apache/geode/pull/4992>
>>>>> 
>>>>> Cheers,
>>>>> Dale
>>>>> 
>>>>> —
>>>>> Dale Emery
>>>>> dem...@pivotal.io
>>>>> dem...@vmware.com
>>>>> 
>>>> 
>>>> 
>>> 
>>> --
>>> Cheers
>>> 
>>> Jinmei
>> 
>> 

—
Dale Emery
dem...@pivotal.io
dem...@vmware.com

Proposal to restore Pulse logging in support/1.12

[Dale Emery]

During the cleanup of the gradle build and logging, the Pulse webapp lost its 
slf4j implementation. As a result, Pulse stopped writing log files.

I’ve restored Pulse logging in the develop branch.

I would like to restore it in support/1.12. I’ve created a PR: 
https://github.com/apache/geode/pull/4992 
<https://github.com/apache/geode/pull/4992>

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io
dem...@vmware.com

[DISCUSS] Prevent locator startup if startup/restart thread throws an uncaught exception

[Dale Emery]

I would like to consider preventing locator startup if a startup or restart 
thread throws an uncaught exception. Otherwise, the cluster can include a 
locator that lacks critical services. We have created 
https://issues.apache.org/jira/browse/GEODE-7775 
<https://issues.apache.org/jira/browse/GEODE-7775> to address this.

We recently observed a serious problem in a user's Geode cluster. The problem 
was enabled by a restart thread's policy of catching uncaught exceptions, 
logging them as "fatal," then exiting the thread without further action.

Here's how the problem happened:

The cluster had 3 locators and 4 servers. An NPE occurred in the "Location 
services restart thread" while a locator was restarting. The thread logged the 
NPE and exited, having never started the configuration persistence service. 
This incomplete locator then joined the cluster.

The user then issued numerous gfsh commands to create, destroy, and re-create 
regions, routing each gfsh command to a different locator in round-robin 
fashion.

Approximately a third of the commands were executed via the incomplete locator. 
Though the commands properly created or destroyed the regions, these results 
were never recorded in the persisted configuration. As a result, the persisted 
configuration was missing definitions for a third of the regions, and had 
duplicate or even triplicate definitions for others.

When the user tried to restart a server, the server detected that the persisted 
configuration was invalid and refused to start.

We have fixed the NPE that initially triggered the problem.

We still have a vulnerability: If in the future a startup/restart thread 
suffers some other exception before it finishes starting its services, the 
thread will log it and exit, allowing the incomplete locator to join the 
cluster.

Some things I don't know:
- What was the reason for instituting the LoggingThread's policy of logging 
exceptions as "fatal" and otherwise ignoring them?
- In which threads should uncaught exceptions prevent startup?
- In which threads should uncaught exceptions be logged and ignored?

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io
dem...@vmware.com

Re: [DISCUSS] Replacing singleton PoolManager

[Dale Emery]

> Dale - are you suggesting a ConnectionPoolService that returns ConnectionPool 
> instances?

Yes.

> Would that mean ConnectionPool would extend Pool and we would deprecate Pool 
> itself?

Maybe extend. I worry about extending, for two reasons.

First, extending would make the new interface depend on the deprecated one. 
That feels awkward for reasons I can’t articulate

Second, extending would mean that the new interface gets all the methods of the 
deprecated one whether we want them or not. I don’t know enough about Pool to 
have an opinion about whether we want to carry all of its method signatures 
forward.

An alternative to consider: Each ConnectionPool implementation delegates to a 
Pool. I suspect that this would make it harder to migrate existing uses from 
Pool to ConnectionPool.

—
Dale Emery
dem...@pivotal.io

Re: [DISCUSS] Replacing singleton PoolManager

[Dale Emery]

+1

To the extent possible without breaking existing APIs, please name the new 
stuff to indicate what’s in the pool (E.g. ConnectionPool, 
ConnectionPoolService, and so on).

—
Dale Emery
dem...@pivotal.io



> On Dec 5, 2019, at 4:40 PM, Dan Smith  wrote:
> 
> Hi,
> 
> I wrote up a proposal for deprecating of the singleton PoolManager in favor
> of a ClientCache scoped service. Please review and comment on the below
> proposal.
> 
> I think this should address the issues that Spring Data Geode and friends
> had trying to mock Pools and remove the need for those projects to try to
> inject mock Pools into a Geode singleton.
> 
> https://cwiki.apache.org/confluence/display/GEODE/Replace+singleton+PoolManager+with+ClientCache+scoped+service
> 
> Thanks,
> -Dan

Re: [DISCUSS] - Move gfsh into its own submodule

[Dale Emery]

+ힹ
—
Dale Emery
dem...@pivotal.io



> On Nov 22, 2019, at 8:39 AM, Jens Deppe  wrote:
> 
> Hello All,
> 
> We'd like to propose moving gfsh and all associated commands into its own
> gradle submodule (implicitly thus also producing a separate maven
> artifact). The underlying intent is to decouple geode core from any Spring
> dependencies.
> 
> The proposal is outlined here:
> https://cwiki.apache.org/confluence/display/GEODE/Move+gfsh+code+to+a+separate+gradle+sub-project
> 
> Please provide feedback for this proposal *on this email thread* and not in
> the comment section of the proposal page.
> 
> The deadline for this proposal will be Monday, December 2.
> 
> Thanks in advance for feedback / comments.
> 
> --Jens & Patrick

Re: Question about excluding serialized classes

[Dale Emery]

> On Sep 16, 2019, at 7:54 PM, Jacob Barrett  wrote:
> 
> The current implementation has statistics without decoration.

For our meters, we want to make a distinction that the current stats 
implementation does not: We want to measure only non-internal functions. It 
isn’t clear (yet) how we can inform the function stats constructor so that it 
knows whether to create the meters.

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io

Re: Question about excluding serialized classes

[Dale Emery]

As far as I can tell, the things that execute functions use the public API to 
find the function to execute. So if we unwrap the functions in the public API, 
only the un-instrumented functions will be executed.

—
Dale Emery
dem...@pivotal.io



> On Sep 11, 2019, at 1:38 PM, Dan Smith  wrote:
> 
> I think you could still use your decorator approach if you also unwrap the
> Functions when returning them from the public APIs like getFunction etc. In
> that case your TimingFunction could probably safely by added to
> excludedClasses.txt.
> 
> You will miss collecting metrics about functions that aren't registered and
> are invoked using Execution.execute(Function) but maybe that's intentional?
> 
> -Dan
> 
> On Wed, Sep 11, 2019 at 1:24 PM Mark Hanson  wrote:
> 
>> They would be the specific functions, but this doesn’t get us around they
>> other problem. I think this approach is not going to work and we are about
>> to start looking into other ways.
>> 
>> Thanks,
>> Mark
>> 
>>> On Sep 11, 2019, at 12:14 PM, Anthony Baker  wrote:
>>> 
>>> I think the Decorator approach you outlined could have other impacts as
>> well.  Would I still be able to see specific function executions in
>> statistics or would they all become “TImingFunction”?
>>> 
>>> Anthony
>>> 
>>> 
>>>> On Sep 11, 2019, at 12:00 PM, Aaron Lindsey 
>> wrote:
>>>> 
>>>> Thanks for your response, Dan.
>>>> 
>>>> The second scenario you mentioned (i.e. users calling
>>>> FunctionService.getFunction(String)) worries me because our PR changes
>> the
>>>> FunctionService so they would now get back an instance of the decorator
>>>> function instead of the specific instance they registered by calling
>>>> FunctionService.registerFunction(Function). Therefore, any explicit
>> casts
>>>> to a Function implementation like (MyFunction)
>>>> FunctionService.getFunction("MyFunction") would fail. Does that mean
>> this
>>>> be a breaking change? The FunctionService class does not specify that
>>>> getFunction must return the same type function as the one passed to
>>>> registerFunction, but I could see how users might be relying on that
>>>> behavior since there is no other way to get a specific function type
>> out of
>>>> the FunctionService without doing a cast.
>>>> 
>>>> - Aaron
>>>> 
>>>> 
>>>> On Wed, Sep 11, 2019 at 10:52 AM Dan Smith  wrote:
>>>> 
>>>>> Functions are serialized when you call Execution.execute(Function)
>> instead
>>>>> of Execution.execute(String). Invoking execute on a function object
>>>>> serializes the function and executes it on the remote side. Functions
>>>>> executed this way don't have be registered.
>>>>> 
>>>>> Users can also get registered function objects directly from the
>> function
>>>>> service using FunctionService.getFunction(String) and do whatever they
>> want
>>>>> with them, which I guess could include serializing them.
>>>>> 
>>>>> Hope that helps!
>>>>> -Dan
>>>>> 
>>>>> On Wed, Sep 11, 2019 at 10:27 AM Aaron Lindsey 
>>>>> wrote:
>>>>> 
>>>>>> As part of a PR to add Micrometer timers for function executions
>>>>>> <https://github.com/apache/geode/pull/4038>, we implemented a
>> decorator
>>>>>> Function that wraps all registered non-internal functions and adds
>>>>>> instrumentation. This PR is
>>>>>> failing AnalyzeSerializablesJUnitTest.testSerializables because the
>>>>>> decorator class is a new Serializable.
>>>>>> 
>>>>>> I'm not sure if it would be OK to add this class to
>> excludedClasses.txt
>>>>>> because I don't know whether this function will ever be serialized.
>> If it
>>>>>> will be serialized, then I'm concerned that this might break backwards
>>>>>> compatibility because we're changing the serialized form of registered
>>>>>> functions. If this is the case, then we could implement custom logic
>> for
>>>>>> serializing the decorator class which would replace its serialized
>> form
>>>>>> with the serialized form of the inner class. Again, I'm not sure if
>> that
>>>>>> would be necessary because I don't know the conditions under which a
>>>>>> function would be serialized.
>>>>>> 
>>>>>> Could someone help me understand when functions would be persisted or
>>>>> sent
>>>>>> over the wire so I can determine if this change would break
>>>>> compatibility?
>>>>>> 
>>>>>> Thanks,
>>>>>> Aaron
>>>>>> 
>>>>> 
>>> 
>> 
>> 

Re: Question about excluding serialized classes

[Dale Emery]

The stats use the ID of the function, and each TimingFunction reports the same 
ID as the function it wraps. So I think the stats would look like they always 
did.

Dale

—
Dale Emery
dem...@pivotal.io



> On Sep 11, 2019, at 12:14 PM, Anthony Baker  wrote:
> 
> I think the Decorator approach you outlined could have other impacts as well. 
>  Would I still be able to see specific function executions in statistics or 
> would they all become “TImingFunction”?
> 
> Anthony
> 
> 
>> On Sep 11, 2019, at 12:00 PM, Aaron Lindsey  wrote:
>> 
>> Thanks for your response, Dan.
>> 
>> The second scenario you mentioned (i.e. users calling
>> FunctionService.getFunction(String)) worries me because our PR changes the
>> FunctionService so they would now get back an instance of the decorator
>> function instead of the specific instance they registered by calling
>> FunctionService.registerFunction(Function). Therefore, any explicit casts
>> to a Function implementation like (MyFunction)
>> FunctionService.getFunction("MyFunction") would fail. Does that mean this
>> be a breaking change? The FunctionService class does not specify that
>> getFunction must return the same type function as the one passed to
>> registerFunction, but I could see how users might be relying on that
>> behavior since there is no other way to get a specific function type out of
>> the FunctionService without doing a cast.
>> 
>> - Aaron
>> 
>> 
>> On Wed, Sep 11, 2019 at 10:52 AM Dan Smith  wrote:
>> 
>>> Functions are serialized when you call Execution.execute(Function) instead
>>> of Execution.execute(String). Invoking execute on a function object
>>> serializes the function and executes it on the remote side. Functions
>>> executed this way don't have be registered.
>>> 
>>> Users can also get registered function objects directly from the function
>>> service using FunctionService.getFunction(String) and do whatever they want
>>> with them, which I guess could include serializing them.
>>> 
>>> Hope that helps!
>>> -Dan
>>> 
>>> On Wed, Sep 11, 2019 at 10:27 AM Aaron Lindsey 
>>> wrote:
>>> 
>>>> As part of a PR to add Micrometer timers for function executions
>>>> <https://github.com/apache/geode/pull/4038>, we implemented a decorator
>>>> Function that wraps all registered non-internal functions and adds
>>>> instrumentation. This PR is
>>>> failing AnalyzeSerializablesJUnitTest.testSerializables because the
>>>> decorator class is a new Serializable.
>>>> 
>>>> I'm not sure if it would be OK to add this class to excludedClasses.txt
>>>> because I don't know whether this function will ever be serialized. If it
>>>> will be serialized, then I'm concerned that this might break backwards
>>>> compatibility because we're changing the serialized form of registered
>>>> functions. If this is the case, then we could implement custom logic for
>>>> serializing the decorator class which would replace its serialized form
>>>> with the serialized form of the inner class. Again, I'm not sure if that
>>>> would be necessary because I don't know the conditions under which a
>>>> function would be serialized.
>>>> 
>>>> Could someone help me understand when functions would be persisted or
>>> sent
>>>> over the wire so I can determine if this change would break
>>> compatibility?
>>>> 
>>>> Thanks,
>>>> Aaron
>>>> 
>>> 
> 

Re: [DISCUSS] Add a public API to add endpoints to Geode's HTTP server

[Dale Emery]

In my proposal as drafted, I assumed Jetty.

If we want to support some additional set of server implementations, we will 
have to define an abstract version of Handler, and perhaps other types, that 
would be adaptable to all of those implementations.

To do that, we would have to have some set of server implementations in mind.

I don’t have any insight into that, so I don’t know how to assess the cost or 
value of making this independent of Jetty.

—
Dale Emery
dem...@pivotal.io



> On Aug 23, 2019, at 9:59 AM, Aaron Lindsey  wrote:
> 
> Would it be practical to remove the dependency on Jetty from the
> HttpService interface? I admit that I don't know a lot about this area of
> the code, but I noticed that the current HttpService public interface
> doesn't have any dependencies on Jetty (except the getHttpService() method,
> which would be changed by this proposal). It seems desirable to decouple
> the interface from the underlying web server in case we need to change it
> later.
> 
> I like this proposal's intent and think it's an important area of work,
> because any Geode user who wants to add a custom HTTP endpoint currently
> has to rely on unstable internal APIs.
> 
> - Aaron
> 
> 
> On Tue, Aug 20, 2019 at 1:34 PM Dale Emery  wrote:
> 
>> I’ve drafted a proposal to add a public API to add endpoints to Geode’s
>> HTTP server.
>> 
>> 
>> https://cwiki.apache.org/confluence/display/GEODE/%5BDraft%5D+Public+API+to+Add+Endpoints+to+Geode+HTTP+Server
>> <
>> https://cwiki.apache.org/confluence/display/GEODE/[Draft]+Public+API+to+Add+Endpoints+to+Geode+HTTP+Server
>> <https://cwiki.apache.org/confluence/display/GEODE/%5BDraft%5D+Public+API+to+Add+Endpoints+to+Geode+HTTP+Server>
>>> 
>> 
>> Currently it is possible to serve additional HTTP content only by creating
>> a separate server or by using internal Geode APIs to add endpoints to the
>> existing server.
>> 
>> We would like to give users a public way to serve additional HTTP content.
>> For example, a user may wish to use a PrometheusMeterRegistry to publish
>> Geode’s Micrometer-based metrics via HTTP. Geode’s existing HTTP server
>> would be a convenient way to publish that information.
>> 
>> I invite your feedback and ideas.
>> 
>> Dale
>> 
>> —
>> Dale Emery
>> dem...@pivotal.io
>> 
>> 

[DISCUSS] Add a public API to add endpoints to Geode's HTTP server

[Dale Emery]

I’ve drafted a proposal to add a public API to add endpoints to Geode’s HTTP 
server.

https://cwiki.apache.org/confluence/display/GEODE/%5BDraft%5D+Public+API+to+Add+Endpoints+to+Geode+HTTP+Server
 
<https://cwiki.apache.org/confluence/display/GEODE/[Draft]+Public+API+to+Add+Endpoints+to+Geode+HTTP+Server>

Currently it is possible to serve additional HTTP content only by creating a 
separate server or by using internal Geode APIs to add endpoints to the 
existing server.

We would like to give users a public way to serve additional HTTP content. For 
example, a user may wish to use a PrometheusMeterRegistry to publish Geode’s 
Micrometer-based metrics via HTTP. Geode’s existing HTTP server would be a 
convenient way to publish that information.

I invite your feedback and ideas.

Dale

—
Dale Emery
dem...@pivotal.io

Re: IntelliJ setup for develop

[Dale Emery]

Running a ‘devBuild’ once on the command line will fix up some modules, 
sometimes. It doesn’t appear to help with running the acceptance tests in 
geode-assembly, but (usually? often?) does allow running geode-core tests in 
IntelliJ.

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io



> On Jul 24, 2019, at 3:37 PM, Murtuza Boxwala  wrote:
> 
> In my ideal world, I compile and run tests with IntelliJ.  IntelliJ is 
> constantly recompiling, so when you hit run, it’s usually just ready to run 
> the tests and the feedback loop feels much faster.
> 
> But, the “output path is not specified for modules” does seem to reappear 
> randomly, and apparently it will disappear on its own after a few restarts 
> maybe…not sure.  A re-import will fix the issue as well, but the 
> unreliability is definitely annoying.  Switching to gradle makes things more 
> predictable at the cost of speed.  I currently have both runners set as 
> "Default (IntelliJ)” and haven’t had issues for a couple weeks now.
> 
>> On Jul 24, 2019, at 5:32 PM, Aaron Lindsey  wrote:
>> 
>> I had the same issue. Now I use the same configuration as Jens and that 
>> fixed the issue. The only problem is that I feel the Gradle build takes 
>> longer than IntelliJ’s build.
>> 
>> - Aaron
>> 
>>> On Jul 24, 2019, at 2:12 PM, Jens Deppe  wrote:
>>> 
>>> I'd suggest not trying to build with IntelliJ, but delegate to Gradle.
>>> (Search for 'gradle' in Settings and then set 'Build and run using 
>>> *Gradle*').
>>> You can still run tests with IntelliJ (this is my preference as I feel it's
>>> faster).
>>> 
>>> On Wed, Jul 24, 2019 at 2:03 PM Sai Boorlagadda 
>>> wrote:
>>> 
>>>> Building/Rebuilding in IntelliJ fails with error "Cannot start compilation:
>>>> the output path is not specified for modules". Any help would be
>>>> appreciated. I have been following BUILDING.md[1] to setup intellij.
>>>> 
>>>> [1] https://github.com/apache/geode/blob/develop/BUILDING.md
>>>> 
>> 
> 

Re: [DISCUSS] RFC 0: Lightweight RFC Process

[Dale Emery]

Hi Dan,

> RFC Process
> <https://cwiki.apache.org/confluence/display/GEODE/Lightweight+RFC+Process>?

I’d like the language to match. For stuff that has been implemented, I like 
Implemented, but Completed is also fine.

Do we have a way to identify proposals for things that are intended to be 
ongoing? An example is the proposal for Instrumenting Geode Code. The intention 
is that it applies each time we add or change instrumentation. It will never be 
Completed.

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io



> On Jul 15, 2019, at 4:52 PM, Dan Smith  wrote:
> 
> @Udo
> 
> I like both of your suggestions. Most of the proposals that I put in
> "Dropped" still seemed like good ideas, perhaps even things we'd already
> agreed to on the mailing list, but hadn't seen any recent development.
> 
> If no one objects, I'll go ahead and rename Active->Completed and
> Dropped->Icebox. Should we change the name of the state in the Lightweight
> RFC Process
> <https://cwiki.apache.org/confluence/display/GEODE/Lightweight+RFC+Process>?
> 
> -Dan
> 
> On Mon, Jul 15, 2019 at 3:57 PM Udo Kohlmeyer  wrote:
> 
>> @Dan,
>> 
>> Thank you for your first attempt at this.
>> 
>> Maybe we should be a rename "Active" to "Completed". "Active" to me
>> means that we are currently working on them, rather having completed
>> them. I don't view these proposals as features that can be toggled
>> on/off (or active/inactive).
>> 
>> Also, I disagree with the approach that proposals that are not actively
>> worked on are "Dropped". Which in itself is incorrect as well. Maybe
>> there should be an "Icebox" area, that lists a set of proposals that
>> have not yet been approved, but also not yet rejected.
>> 
>> I think it is ok to have an "Icebox" of proposals that lists areas of
>> improvement we want to target, but as of yet, no concrete proposal has
>> yet been submitted. Modularity comes to mind. It is not that we don't
>> want to do it, it is just that there is no proposal that has been
>> accepted/completed.
>> 
>> --Udo
>> 
>> On 7/12/19 12:57, Dan Smith wrote:
>>> Following up on this, I took a stab at organizing our old proposals into
>>> the buckets on the wiki. We now have:
>>> 
>>> Under Discussion - Draft and In Discussion proposals
>>> In Development - proposals under active development
>>> Active - Proposals that are completely implemented
>>> Dropped - Proposals that were not approved or development stalled out.
>>> 
>>> If I moved your proposal to "Dropped" erroneously, please feel free to
>> move
>>> it back! I moved things there that did not appear to have been
>> implemented
>>> or have any recent activity.
>>> 
>>> I put a few things in "Unknown State." If you know what state these
>>> proposals are in, please move them!
>>> 
>>> 
>> https://cwiki.apache.org/confluence/display/GEODE/Project+Proposals+and+Specifications
>>> 
>>> On Wed, Jun 26, 2019 at 11:20 AM Alexander Murmann 
>>> wrote:
>>> 
>>>> Given discussion here and previous discussion on the PR, I consider this
>>>> proposal approved and updated its state accordingly.
>>>> 
>>>> I also incorporated Dan's suggestion of moving deprecated proposals and
>>>> added a reference to the new process at the top of the Project Proposals
>>>> and Specifications page
>>>> <
>>>> 
>> https://cwiki.apache.org/confluence/display/GEODE/Project+Proposals+and+Specifications
>>>> .
>>>> 
>>>> Thank you all for you great feedback throughout this process!
>>>> 
>>>> On Tue, Jun 25, 2019 at 10:07 AM Dan Smith  wrote:
>>>> 
>>>>>> Will moving the page around on the wiki result in dead links to the
>>>> draft
>>>>>> version?
>>>>>> 
>>>>> No. If you use the share button in the wiki, you get a permanent link
>> to
>>>>> the page. Even if you just copy the URL from the address bar it doesn't
>>>>> include the folder the page is in.
>>>>> 
>>>>> -Dan
>>>>> 
>>>> 
>>>> --
>>>> Alexander J. Murmann
>>>> (650) 283-1933
>>>> 
>> 

Re: Unnecessary uses of final on local variables

[Dale Emery]

> On Jun 13, 2019, at 4:15 PM, Ryan McMahon  wrote:
> 
> I agree with this sentiment, and have generally only been using final on
> class fields and method parameters where I want to guarantee immutability
> as of late.  However, I was at one time adding final to local variables,
> and I know that I have checked in code with final locals.  Should we
> actively be removing finals when we see it on locals?
> 
> One case where I still have been using final for locals is when the
> variable is effectively constant and I want to show that intent.  For
> instance:
> final String resourceId =
> "someStaticResourceIdThatReallyShouldntBeReassignedEver";
> 
> Is this a valid use case or should we view this as unnecessary verbosity as
> well?

My preference (fairly strong) is not to mark locals and parameters as final, 
but I won’t go so far as to say it’s invalid to do so.

I would like to know more about the intent, so …

If such a local variable were not marked final, what bad thing do you think 
might happen?

A more positive version of the same question: If you’re able to show that the 
variable is effectively constant, what good things does that do for you?

—
Dale Emery
dem...@pivotal.io

Re: Unnecessary uses of final on local variables

[Dale Emery]

> On Jun 13, 2019, at 4:29 PM, Juan José Ramos  wrote:
> 
> +1 to removing *final* on local variables.
> However, regarding Ryan's example, and even if it adds some "noise" to the
> source code, I'm in favour of keeping the *final* keyword on local
> variables whenever the developer wants to explicitly show the intent of
> making that the variable effectively constant.

How will we know whether it’s an explicit intention, vs an old habit or 
something else?

—
Dale Emery
dem...@pivotal.io



> On Jun 13, 2019, at 4:29 PM, Juan José Ramos  wrote:
> 
> +1 to removing *final* on local variables.
> However, regarding Ryan's example, and even if it adds some "noise" to the
> source code, I'm in favour of keeping the *final* keyword on local
> variables whenever the developer wants to explicitly show the intent of
> making that the variable effectively constant.
> Cheers.
> 
> 
> On Fri, Jun 14, 2019 at 12:15 AM Ryan McMahon  wrote:
> 
>> I agree with this sentiment, and have generally only been using final on
>> class fields and method parameters where I want to guarantee immutability
>> as of late.  However, I was at one time adding final to local variables,
>> and I know that I have checked in code with final locals.  Should we
>> actively be removing finals when we see it on locals?
>> 
>> One case where I still have been using final for locals is when the
>> variable is effectively constant and I want to show that intent.  For
>> instance:
>> final String resourceId =
>> "someStaticResourceIdThatReallyShouldntBeReassignedEver";
>> 
>> Is this a valid use case or should we view this as unnecessary verbosity as
>> well?
>> 
>> Thanks,
>> Ryan
>> 
>> 
>> 
>> On Thu, Jun 13, 2019 at 1:31 PM Kirk Lund  wrote:
>> 
>>> According to Effective Java 3rd Edition, all local variables are
>> implicitly
>>> made final by the JVM (and thus receiving the same JVM optimizations as a
>>> final variable) without requiring use of the keyword as long as you only
>>> set the value once. So this becomes an unnecessary use of the keyword
>>> final which
>>> really just adds noise to the code (one more word on many lines) much
>> like
>>> unnecessary uses of this. on references to class fields. The only context
>>> where it's still valuable to use final is on class fields and constants
>>> where it provides concurrency guarantees.
>>> 
>>> It may be useful to temporarily mark a local variable as final while
>>> performing a refactoring. See Martin Fowler's book for various
>> refactorings
>>> in which he does this.
>>> 
>>> There really is no value in retaining the keyword on a local variable any
>>> longer, so I think we should avoid using it in the same way we avoid
>>> unnecessary uses of this. on references to class fields. It's all just
>> more
>>> noise in the code.
>>> 
>> 
> 
> 
> -- 
> Juan José Ramos Cassella
> Senior Technical Support Engineer
> Email: jra...@pivotal.io
> Office#: +353 21 4238611
> Mobile#: +353 87 2074066
> After Hours Contact#: +1 877 477 2269
> Office Hours: Mon - Thu 08:30 - 17:00 GMT. Fri 08:30 - 16:00 GMT
> How to upload artifacts:
> https://support.pivotal.io/hc/en-us/articles/204369073
> How to escalate a ticket:
> https://support.pivotal.io/hc/en-us/articles/203809556
> 
> [image: support] <https://support.pivotal.io/> [image: twitter]
> <https://twitter.com/pivotal> [image: linkedin]
> <https://www.linkedin.com/company/3048967> [image: facebook]
> <https://www.facebook.com/pivotalsoftware> [image: google plus]
> <https://plus.google.com/+Pivotal> [image: youtube]
> <https://www.youtube.com/playlist?list=PLAdzTan_eSPScpj2J50ErtzR9ANSzv3kl>

Re: [DISCUSS] Proposal to re-cut Geode 1.9.0 release branch

[Dale Emery]

> On Mar 20, 2019, at 11:25 AM, Alexander Murmann  wrote:
> 
> Dale, is there any downside to making these changes in 1.10 other than
> plainly having them later?

I don’t think so, but I’d want to hear Dan’s opinion on that, given that his 
approval of our PR was contingent on our promise to do it.

FYI, the additional work to improve usability is non-trivial, which is why we 
haven’t done it already.

—
Dale Emery
dem...@pivotal.io



> On Mar 20, 2019, at 11:25 AM, Alexander Murmann  wrote:
> 
> Dale, is there any downside to making these changes in 1.10 other than
> plainly having them later?
> 
> On Wed, Mar 20, 2019 at 11:15 AM Dave Barnes  wrote:
> 
>> geode-native is ready to into the 1.9 release candidate build.
>> 
>> On Wed, Mar 20, 2019 at 10:42 AM Dave Barnes  wrote:
>> 
>>> The geode-native PR will be ready to check in momentarily. Just waiting
>>> for Travis to do its diligence.
>>> 
>>> On Wed, Mar 20, 2019 at 9:47 AM Alexander Murmann 
>>> wrote:
>>> 
>>>> Dale, do I understand correctly that the only concern around the
>>>> Micrometer
>>>> work right now it that it's not useful yet, however it's not harmful
>>>> either?
>>>> 
>>>> Dave, is it correct that if that PR doesn't make it into the newly cut
>>>> branch, we'd be shipping with a older version of geode-native? What are
>>>> the
>>>> two versions and what would be the implications of this not making it
>> into
>>>> this release?
>>>> 
>>>> On Tue, Mar 19, 2019 at 5:29 PM Dave Barnes  wrote:
>>>> 
>>>>> The Geode 1.9.0 release includes a source-only release of the
>>>> geode-native
>>>>> repo. There's a pull-request in process to update version numbers and
>>>> the
>>>>> doc build environment in that repo; should be ready to merge tomorrow
>>>>> morning.
>>>>> 
>>>>> On Tue, Mar 19, 2019 at 5:20 PM Dale Emery  wrote:
>>>>> 
>>>>>> The Micrometer API is in, and marked as experimental. But we have
>> not
>>>> yet
>>>>>> updated CacheFactory to allow injecting a meter registry (or metrics
>>>>>> publishing service) there. So currently the only way to publish is
>> to
>>>> add
>>>>>> metrics publishing service via the ServiceLoader mechanism.
>>>>>> 
>>>>>> —
>>>>>> Dale Emery
>>>>>> dem...@pivotal.io
>>>>>> 
>>>>>> 
>>>>>>> On Mar 19, 2019, at 3:29 PM, Dan Smith  wrote:
>>>>>>> 
>>>>>>> Is the geode-managability sub-project and the new micrometer API
>> in
>>>> a
>>>>>> place
>>>>>>> where we can cut a release branch? I know a bunch of changes have
>>>> gone
>>>>> in
>>>>>>> since the release branch, are we comfortable releasing these new
>>>>>>> experimental features as they are right now?
>>>>>>> 
>>>>>>> -Dan
>>>>>>> 
>>>>>>> On Tue, Mar 19, 2019 at 2:38 PM Dick Cavender 
>>>>> wrote:
>>>>>>> 
>>>>>>>> +1 to re-cutting the 1.9 release branch off a more stable develop
>>>> sha
>>>>>>>> within the last couple days.
>>>>>>>> 
>>>>>>>> On Tue, Mar 19, 2019 at 1:14 PM Bruce Schuchardt <
>>>>>> bschucha...@pivotal.io>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> If we recut the release branch we need to update JIRA tickets
>>>> marked
>>>>>>>>> fixed in 1.10
>>>>>>>>> 
>>>>>>>>> On 3/19/19 12:48 PM, Sai Boorlagadda wrote:
>>>>>>>>>>> It was known at the time that develop was not as stable as
>>>> desired,
>>>>>>>>>> so we planned to cherry-pick fixes from develop until the
>> release
>>>>>>>>>> branch was stable enough to ship.
>>>>>>>>>> I want to clarify that we decided to cut the release branch not
>>>> that
>>>>>>>>>> develop was not stable. But really that it is desirable to cut
>>>> the
>>>>>>>>>> branch soon

Re: [DISCUSS] Proposal to re-cut Geode 1.9.0 release branch

[Dale Emery]

Hi Alexander,


> On Mar 20, 2019, at 9:47 AM, Alexander Murmann  wrote:
> 
> Dale, do I understand correctly that the only concern around the Micrometer
> work right now it that it's not useful yet, however it's not harmful either?

It’s useful, but somewhat less usable than we want it to be. Currently the only 
way to publish the metrics is to implement a service loader. Dan suggested that 
we offer additional ways that are easier for Geode users. For example, passing 
a “publishing registry” to the cache factory. We think that’s a great idea, and 
we promised to do it, but haven’t done it yet.

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io

Re: [DISCUSS] Proposal to re-cut Geode 1.9.0 release branch

[Dale Emery]

The Micrometer API is in, and marked as experimental. But we have not yet 
updated CacheFactory to allow injecting a meter registry (or metrics publishing 
service) there. So currently the only way to publish is to add metrics 
publishing service via the ServiceLoader mechanism.

—
Dale Emery
dem...@pivotal.io


> On Mar 19, 2019, at 3:29 PM, Dan Smith  wrote:
> 
> Is the geode-managability sub-project and the new micrometer API in a place
> where we can cut a release branch? I know a bunch of changes have gone in
> since the release branch, are we comfortable releasing these new
> experimental features as they are right now?
> 
> -Dan
> 
> On Tue, Mar 19, 2019 at 2:38 PM Dick Cavender  wrote:
> 
>> +1 to re-cutting the 1.9 release branch off a more stable develop sha
>> within the last couple days.
>> 
>> On Tue, Mar 19, 2019 at 1:14 PM Bruce Schuchardt 
>> wrote:
>> 
>>> If we recut the release branch we need to update JIRA tickets marked
>>> fixed in 1.10
>>> 
>>> On 3/19/19 12:48 PM, Sai Boorlagadda wrote:
>>>>> It was known at the time that develop was not as stable as desired,
>>>> so we planned to cherry-pick fixes from develop until the release
>>>> branch was stable enough to ship.
>>>> I want to clarify that we decided to cut the release branch not that
>>>> develop was not stable. But really that it is desirable to cut the
>>>> branch sooner to avoid any regression risk that can be introduced by
>>>> on-going work on develop.
>>>> 
>>>> Nevertheless looks like develop is more stable than release branch due
>>>> to some test fixes that were not cherry-picked into the release branch.
>>>> I think its a good idea to re-cut the branch as our current position
>>>> to stabilize release branch before releasing.
>>>> 
>>>> +1 to re-cut.
>>>> 
>>>> Sai
>>>> 
>>>> On Tue, Mar 19, 2019 at 12:19 PM Owen Nichols >>> <mailto:onich...@pivotal.io>> wrote:
>>>> 
>>>>The Geode 1.9.0 release branch was originally cut 4 weeks ago on
>>>>Feb 19.  It was known at the time that develop was not as stable
>>>>as desired, so we planned to cherry-pick fixes from develop until
>>>>the release branch was stable enough to ship.  While this is a
>>>>good strategy when starting from a fairly good baseline, it seems
>>>>in this case it has only added complexity without leading to
>>>>stability.
>>>> 
>>>>Looking at the pipelines over the last week (see attached
>>>>metrics), it appears we have been far more successful at
>>>>stabilizing /develop/ than /release/1.9.0/. Rather than trying to
>>>>cherry-pick more and more fixes to the release branch, I propose
>>>>we RE-CUT the 1.9.0 release branch later this week in order to
>>>>start from a much more stable baseline.
>>>> 
>>>>-Owen
>>>> 
>>>> 
>>>> 
>>> 
>> 

Re: [Proposal] Adding Micrometer to Apache Geode

[Dale Emery]

I have submitted a new PR https://github.com/apache/geode/pull/3180 
<https://github.com/apache/geode/pull/3180>, superseding the original one.

I invite your review.

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io

> On Feb 4, 2019, at 9:10 AM, Kirk Lund  wrote:
> 
> +1 to add Micrometer in the described way and I'll add my approval to the
> PR as well
> 
> On Fri, Feb 1, 2019 at 4:16 PM Dale Emery  wrote:
> 
>> Hello all,
>> 
>> I've created a PR to add Micrometer to Geode:
>> https://github.com/apache/geode/pull/3153
>> 
>> I invite your review.
>> 
>> The Micrometer system includes a rich suite of Meter types for
>> instrumenting code, and several styles of meter registries for
>> maintaining the meters.  It also includes (as separate jars) additional
>> registry types for publishing metrics to a variety of external
>> monitoring systems (e.g. Prometheus, JMX, and Influx).
>> 
>> See http://micrometer.io/ for details about Micrometer,
>> 
>> This PR adds a Micrometer-based metrics system to Geode.
>> 
>> On startup, Geode configures a "primary" meter registry. Developers use
>> the primary registry to create meters (gauges, counters, timers, and
>> others) to instrument Geode and client code.
>> 
>> Internally, the meter registry is a "composite" registry, which allows
>> attaching any number of "downstream" registries. Each downstream
>> registry will typically publish the metrics to an external monitoring
>> system, or provide an endpoint where an external monitoring system can
>> "scrape" the metrics.
>> 
>> In Geode, when a meter is added or removed from the primary registry,
>> the primary registry adds or removes a corresponding meter in each
>> downstream registry.  When a meter in the primary registry is updated
>> (e.g. incremented), the primary registry forwards each operation to the
>> corresponding meter in each downstream registry.
>> 
>> A MetricsCollector provides access to the primary registry, and includes
>> methods for adding and removing downstream registries.
>> 
>> The MetricsCollector itself is currently available via a method on
>> InternalDistributedSystem.  Eventually we plan to  make the
>> MetricsCollector available through DistributedSystem, or some other
>> non-internal class or interface.
>> 
>> The key components of this change are:
>> - MetricsCollector (interface) allows access to the primary registry,
>>  and allows adding and removing downstream registries.
>> - CompositeMetricsCollector is the concrete implementation of
>>  MetricsCollector, and creates the internal composite registry.
>> - InternalDistributedSystem creates the MetricsCollector and makes it
>>  available via a new getMetricsCollector() method.
>> - The Micrometer system for instrumenting and maintaining metrics (added
>>  to Geode as a dependency).
>> 
>> Cheers,
>> Dale
>> 
>>> On Jan 15, 2019, at 9:37 AM, Mark Hanson  wrote:
>>> 
>>> Hello All,
>>> 
>>> I would like to propose that we incorporate Micrometer into Geode to
>> allow us to collect statistics and make them more easily available to
>> external services should someone chose to implement support for that.
>>> 
>>> In some basic testing, it does not appear to have a significant impact
>> on performance to hook this in. I think that in the long run it will really
>> improve the visibility of the systems stats in real-time… This will also
>> allow some extensibility for people who want to hook into their tracking
>> infrastructure, such as New Relic or Data Dog, though we are not putting
>> that in.
>>> 
>>> 
>>> What do people think?
>>> 
>>> Thanks,
>>> Mark
>>> 
>>> 
>> 
>> 

Re: [Proposal] Adding Micrometer to Apache Geode

[Dale Emery]

Hello all,

I've created a PR to add Micrometer to Geode: 
https://github.com/apache/geode/pull/3153

I invite your review.

The Micrometer system includes a rich suite of Meter types for
instrumenting code, and several styles of meter registries for
maintaining the meters.  It also includes (as separate jars) additional
registry types for publishing metrics to a variety of external
monitoring systems (e.g. Prometheus, JMX, and Influx).

See http://micrometer.io/ for details about Micrometer,

This PR adds a Micrometer-based metrics system to Geode.

On startup, Geode configures a "primary" meter registry. Developers use
the primary registry to create meters (gauges, counters, timers, and
others) to instrument Geode and client code.

Internally, the meter registry is a "composite" registry, which allows
attaching any number of "downstream" registries. Each downstream
registry will typically publish the metrics to an external monitoring
system, or provide an endpoint where an external monitoring system can
"scrape" the metrics.

In Geode, when a meter is added or removed from the primary registry,
the primary registry adds or removes a corresponding meter in each
downstream registry.  When a meter in the primary registry is updated
(e.g. incremented), the primary registry forwards each operation to the
corresponding meter in each downstream registry.

A MetricsCollector provides access to the primary registry, and includes
methods for adding and removing downstream registries.

The MetricsCollector itself is currently available via a method on
InternalDistributedSystem.  Eventually we plan to  make the
MetricsCollector available through DistributedSystem, or some other
non-internal class or interface.

The key components of this change are:
- MetricsCollector (interface) allows access to the primary registry,
  and allows adding and removing downstream registries.
- CompositeMetricsCollector is the concrete implementation of
  MetricsCollector, and creates the internal composite registry.
- InternalDistributedSystem creates the MetricsCollector and makes it
  available via a new getMetricsCollector() method.
- The Micrometer system for instrumenting and maintaining metrics (added
  to Geode as a dependency).

Cheers,
Dale

> On Jan 15, 2019, at 9:37 AM, Mark Hanson  wrote:
> 
> Hello All,
> 
> I would like to propose that we incorporate Micrometer into Geode to allow us 
> to collect statistics and make them more easily available to external 
> services should someone chose to implement support for that.
> 
> In some basic testing, it does not appear to have a significant impact on 
> performance to hook this in. I think that in the long run it will really 
> improve the visibility of the systems stats in real-time… This will also 
> allow some extensibility for people who want to hook into their tracking 
> infrastructure, such as New Relic or Data Dog, though we are not putting that 
> in.
> 
> 
> What do people think?
> 
> Thanks,
> Mark
> 
> 

Re: [Proposal] Adding Micrometer to Apache Geode

[Dale Emery]

Hi Udo and all,

> On Jan 15, 2019, at 10:06 AM, Udo Kohlmeyer  wrote:
> 
> It would be good to see the new Micrometer stats have a logical grouping, 
> that makes it easier for users to search for metrics.

Do you know of any useful guidelines or conventions for creating a hierarchy of 
metrics, and criteria for evaluating the goodness of the hierarchy?

And for which details to represent in the meter name hierarchy vs 
tags/dimensions?

Dale

—
Dale Emery
dem...@pivotal.io

Defining public SPIs in Geode

[Dale Emery]

We are exploring adding one or more public Service Provider Interfaces (SPIs) 
for Geode, and would like some guidance about standards, conventions, 
precedent, and such.

Do we have standards or conventions for creating SPIs? Good examples? Bad 
examples?

Are there standards or conventions described elsewhere (e.g. Spring) that we 
should consider when defining SPIs?

Is it as simple(!) as defining a service interface, then using Java's 
ServiceLoader mechanism to discover and load instances of it?

Any additional considerations for adding a public SPI, compared to a private 
one?

Cheers,
Dale

Re: [DISCUSS] test code style (particularly logging)

[Dale Emery]

In general I agree.

I’m an author of the PR that triggered this thread. That PR specifically prints 
new information to help diagnose an intermittent CI failure in a 
DistributedTest test, which we have been unable to reproduce outside of CI. Our 
working hypothesis is that an action initiated at the end of a test sometimes 
continues beyond the end of the test, interfering with the next test. The 
information we’re printing is designed to help test our hypothesis.

In another case, I added printouts to report progress through a multi-threaded 
“unit” test, where the test establishes a specific interleaving between 
threads. That test also was difficult to reproduce outside of CI. We’re 
confident that we’ve fixed the original problem in the test, but if we’re 
wrong, the printouts give essential information to whoever has to diagnose the 
next failure in CI.

In each of these cases, we originally used logging to write the new 
information. In response to Galen’s comments and other advice in this thread, 
we’ve switched to printouts, to reduce the tests’ unnecessary dependence on 
product code that isn’t relevant to the test.

All of that said, I also strongly prefer tests that are small enough, focused 
enough, and clear enough that logs/printouts are unnecessary.

—
Dale Emery
dem...@pivotal.io

> On Sep 21, 2018, at 10:34 AM, Kirk Lund  wrote:
> 
> Most of these logWriter or logger usages are in larger end-to-end tests
> that were written before we could using IDE debuggers on our tests. With a
> debugger, I don't want to see more output from the test so I tend to delete
> all such System.out.printlns or LogWriter/Logger usage.
> 
> My recommendation is to delete all extra System.out.printlns AND
> LogWriter/Logger usages unless it's part of the test itself. If you think
> the line is useful, then would it be sufficient as a comment? If not then
> maybe the test is too long, doing too much and should be exploded into
> several easier to debug test methods with more narrow focus.
> 
> Remember, a single test that does a lot and has tons of assertions is an
> anti-pattern. It's better to split that test out into many tests such that
> by seeing that the test failed, you either know exactly what broke without
> digging too much or at least have a pretty good idea where to dig. Sure,
> there's a balance to strike especially in end-to-end tests but if the test
> method scrolls along for many screens, then most likely it's doing too much
> and the author(s) added logging to compensate for this.
> 
> On Thu, Sep 20, 2018 at 2:10 PM, Darrel Schneider 
> wrote:
> 
>> For simple single threaded tests System.out would do the job.
>> For a multi-threaded test I have found the logging framework to be helpful
>> because of the thread id and the timestamps.
>> 
>> 
>> On Thu, Sep 20, 2018 at 1:50 PM Dale Emery  wrote:
>> 
>>> As long as the stdout is available in the test results, I’m more than
>>> happy to avoid coupling the tests to the product logging code.
>>> 
>>>> On Sep 20, 2018, at 1:39 PM, Galen O'Sullivan 
>>> wrote:
>>>> 
>>>> I was reviewing a PR recently and noticed that we have some test code
>>> that
>>>> uses Logger (or LogWriter). If I understand correctly, anything logged
>> to
>>>> stdout will be included in the test output, and anything logged in a
>>> DUnit
>>>> VM will be logged with the appropriate VM number prepended in the
>> output.
>>>> Because logging is coupled to product code, I propose we log all test
>>>> output to standard out (using System.out.println or such). I also
>> propose
>>>> we add this to the Geode style guide.
>>>> 
>>>> Thoughts/questions/objections?
>>>> 
>>>> Thanks,
>>>> Galen
>>> 
>>> 
>>> —
>>> Dale Emery
>>> dem...@pivotal.io
>>> 
>>> 
>> 

Re: [DISCUSS] test code style (particularly logging)

[Dale Emery]

As long as the stdout is available in the test results, I’m more than happy to 
avoid coupling the tests to the product logging code.

> On Sep 20, 2018, at 1:39 PM, Galen O'Sullivan  wrote:
> 
> I was reviewing a PR recently and noticed that we have some test code that
> uses Logger (or LogWriter). If I understand correctly, anything logged to
> stdout will be included in the test output, and anything logged in a DUnit
> VM will be logged with the appropriate VM number prepended in the output.
> Because logging is coupled to product code, I propose we log all test
> output to standard out (using System.out.println or such). I also propose
> we add this to the Geode style guide.
> 
> Thoughts/questions/objections?
> 
> Thanks,
> Galen


—
Dale Emery
dem...@pivotal.io

Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.

[Dale Emery]

If there’s enough duplication in the lambdas, or in the code around the 
lambdas, extracting the duplication into methods would reduce the number of 
lambdas.

> On Sep 10, 2018, at 11:03 AM, Kirk Lund  wrote:
> 
> Alright I'm more up-to-date on this thread. Some things to consider:
> 
> The lambdas look great, but we'd have to start splitting the Geode clasess.
> They're so big right now, that if you change a bunch of log statements to
> use lambdas you'll hit the max number of lambdas on many of our classes. We
> hit the lambda limit on a dunit test and it didn't really take that many
> lambdas to hit the limit (see
> https://issues.apache.org/jira/browse/GEODE-5485).
> 
> We could change FastLogger to override every Logger method instead of
> isDebugEnabled and isTraceEnabled. Then we could remove every single guard
> clause from the Geode log statements.
> 
> If Log4J2 changed their implementation of debug and trace to be comparable
> with hitting a volatile for disabled statements then we could delete
> FastLogger and every log statement guard clause.
> 
> On Mon, Sep 10, 2018 at 10:12 AM, Kirk Lund  wrote:
> 
>> The reason we use these guards is that the Log4j2 implementation is more
>> expensive than hitting a volatile. Please don't change anything unless you
>> start writing lots of JMH benchmarks! The existing code went through lots
>> of iterations of perf testing that isn't part of Geode.
>> 
>> Every Log4j2 Logger used by Geode classes are wrapped inside a FastLogger
>> which uses a single volatile for those "isDebugEnabled" checks. If you
>> remove this, you will find that the performance of Geode will thank even at
>> higher log levels such as INFO. Geode is currently optimized for INFO level
>> logging. Without FastLogger, every log statement code path goes down into a
>> hot mess of checking filters and performing file checking against
>> log4j2.xml (based on a mod so that it only occurs every several log
>> statements) to see if it has changed.
>> 
>> Despite this, Log4J2 Core still out performs Logback for "disabled log
>> statements" and by this I mean the huge # of debug/trace level statements
>> in Geode when running at INFO level.
>> 
>> Unwrapping those "isDebugEnabled" checks will eliminate the optimization
>> provided by FastLogger. Without the guard clauses, FastLogger doesn't
>> improve anything because the Log4j2 code skips the "isDebugEnabled" checks
>> and goes into filter checking which also checks the log level but after
>> much more work. My recommendation is to work with Log4j2 project to fix
>> this performance problem when using log statements without the FastLogger
>> optimizations. For example, Log4j1 used a dedicated async thread for the
>> checking of the config file but for some reason they didn't go this route
>> in Log4J2.
>> 
>> PS: My Log4J2 code knowledge is a couple of years old so please feel free
>> to dig into their code to see if the above issues were fixed.
>> 
>> On Mon, Sep 10, 2018 at 9:35 AM, Galen O'Sullivan 
>> wrote:
>> 
>>> I think that logging in hot paths/loops is probably something that should
>>> be avoided. And if it is necessary, I suspect that the JIT will
>>> short-circuit that call since debug levels don't generally change.
>>> 
>>> There probably are a few hot paths that need to optimize logging, but
>>> that's not the majority.
>>> 
>>> Can we agree to avoid this pattern in new code, since it adversely affects
>>> readability?
>>> 
>>> Galen
>>> 
>>> 
>>> On Fri, Sep 7, 2018 at 1:46 PM, Nicholas Vallely 
>>> wrote:
>>> 
 I was just randomly looking into this a couple of days ago as a tangent
>>> to
 'observability' and came across this Stack Overflow:
 https://stackoverflow.com/questions/6504407/is-there-a-need-
>>> to-do-a-iflog-
 isdebugenabled-check
 where the first answer is the most succinct in my opinion.
 
 In the geode code that I searched, we are not consistent with our use of
 the if(statement) wrapper for debug, though most do have the wrapper.
 
 Nick
 
 On Fri, Sep 7, 2018 at 11:35 AM Jacob Barrett 
>>> wrote:
 
> Checking with logger.isDebugEnabled() it still a good practice for hot
> paths to avoid the construction of intermediate object arrays to hold
>>> the
> var-args. Some logging libraries have fixed argument optimizations for
> var-args up to a specific limit. In very hot paths it is nice to
> check logger.isDebugEnabled() once into a temp boolean value and then
 check
> that in all the subsequent debug logging statements in the hot path.
> 
> -Jake
> 
> 
> On Fri, Sep 7, 2018 at 11:18 AM Dan Smith  wrote:
> 
>> I think this pattern is a holdover from using log4j 1. In that
>>> version,
> you
>> added an if check to avoid unnecessary string concatenation if debug
 was
>> enabled.
>> 
>> 
>>   1. if (logger.isDebugEnabled()) {
>>   2. logger.debug("Logging in user " + 

Re: [DISCUSS] Apache Geode 1.7.0 release branch created

[Dale Emery]

I have resolved GEODE-5254

Dale

> On Aug 31, 2018, at 3:34 PM, Nabarun Nag  wrote:
> 
> Requesting status update on the following JIRA tickets. These tickets have
> commits into develop against its name but the status is still open /
> unresolved.
> 
> GEODE-5600 - [Patrick Rhomberg]
> GEODE-5578 - [Robert Houghton]
> GEODE-5492 - [Robert Houghton]
> GEODE-5280 - [xiaojian zhou & Biju Kunjummen]
> GEODE-5254 - [Dale Emery]
> 
> GEODE-4794 - [Sai]
> GEODE-5594 - [Sai]
> 
> Regards
> Nabarun Nag
> 
> 
> On Fri, Aug 31, 2018 at 3:18 PM Nabarun Nag  wrote:
> 
>> 
>> Please continue using 1.7.0 as a fix version in JIRA till the email comes
>> in that the 1.7.0 release branch has be cut.
>> 
>> Changing the fixed version for the following tickets to 1.7.0 from 1.8.0
>> as these fixes will be included in the 1.7.0 release
>> 
>> GEODE-5671
>> GEODE-5662
>> GEODE-5660
>> GEODE-5652
>> 
>> Regards
>> Nabarun Nag
>> 
>> 
>> On Fri, Aug 31, 2018 at 2:20 PM Nabarun Nag  wrote:
>> 
>>> A new feature of get/set cluster config was added as new feature to gfsh.
>>> This needs to be added to the documentation.
>>> Once this is done, the branch will be ready.
>>> 
>>> Regards
>>> Nabarun
>>> 
>>> 
>>> On Fri, Aug 31, 2018 at 2:15 PM Alexander Murmann 
>>> wrote:
>>> 
>>>> Nabarun, do you still see anything blocking cutting the release at this
>>>> point?
>>>> 
>>>> Maybe we can even get a pipeline going today? 
>>>> 
>>>> On Fri, Aug 31, 2018 at 10:38 AM, Sai Boorlagadda <
>>>> sai.boorlaga...@gmail.com
>>>>> wrote:
>>>> 
>>>>> We can go ahead and cut 1.7 with out GEODE-5338 as I don't have the
>>>> code
>>>>> ready.
>>>>> 
>>>>> GEODE-5594, adds a new flag to enable hostname validation and is
>>>> disabled
>>>>> by default so we are good with changes that are already merged and
>>>>> documentation for GEODE-5594 is ready merged.
>>>>> 
>>>>> Naba, after the branch is cut we should delete windows jobs from the
>>>> branch
>>>>> before we create the pipeline for 1.7.
>>>>> 
>>>>> Apologies for holding up the release.
>>>>> 
>>>>> Sai.
>>>>> 
>>>>> On Fri, Aug 31, 2018, 10:23 AM Nabarun Nag  wrote:
>>>>> 
>>>>>> I am waiting on the documentation tickets to get closed before
>>>> cutting
>>>>> the
>>>>>> branch.
>>>>>> 
>>>>>> Regards
>>>>>> Nabarun Nag
>>>>>> 
>>>>>> On Fri, Aug 31, 2018 at 10:18 AM Anthony Baker 
>>>>> wrote:
>>>>>> 
>>>>>>> Perhaps we should cut 1.7.0 without these changes to give us more
>>>> time
>>>>> to
>>>>>>> review and complete the work.
>>>>>>> 
>>>>>>> Thoughts?
>>>>>>> 
>>>>>>> Anthony
>>>>>>> 
>>>>>>> 
>>>>>>>> On Aug 31, 2018, at 8:03 AM, Sai Boorlagadda <
>>>>>> sai.boorlaga...@gmail.com>
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> I haven't yet merged GEODE-5338. The PR changes the existing
>>>> behavior
>>>>>> and
>>>>>>>> is not acceptable.
>>>>>>>> Working on changing the implementation to have a default value
>>>>> derived
>>>>>>>> based on how user
>>>>>>>> wants to configure SSL.
>>>>>>>> 
>>>>>>>> Sai
>>>>>>>> 
>>>>>>>> On Wed, Aug 29, 2018 at 11:45 AM Sai Boorlagadda <
>>>>>>> sai.boorlaga...@gmail.com>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> I have merged GEODE-5594 to develop.
>>>>>>>>> 
>>>>>>>>> GEODE-5338 is now waiting for PR review and precheckin.
>>>>>>>>> 
>>>>>>>>> Sai
>>>>>>>>> 
>>>>>>>>> On Tue, Aug 28, 2018 at 10:30 AM Sai Boorlagadda <
>>>

Re: [DISCUSS] Which assert is the right one to use moving forward?

[Dale Emery]

Between JUnit and AssertJ, my preference is AssertJ, for two reasons. First is 
AssertJ’s pervasiveness in Geode. Second, it decouples assertions from the 
testing framework (in support of my secret desire to move to JUnit 5).

Cheers,
Dale

> On Aug 20, 2018, at 11:49 AM, Mark Hanson  wrote:
> 
> Hi All,
> 
> In the course fo fixing some tests, I have found that the existing Geode 
> asserts are deprecated. In wanting to leave the code as clean of deprecations 
> as possible, I have come to the inevitable quandary.  Which Assert should we 
> be using? JUnit or Assertj? I am happy with either though I will note that 
> JUnit Assert does not seem to have a fail where you can pass in the 
> exception, which is used a lot in Geode. I look forward to an answer.
> 
> Thanks,
> Mark
> 
> 

—
Dale Emery
dem...@pivotal.io

Re: Proposed API for a ConcurrencyTestHelper

[Dale Emery]

e loop will not restart
>   * the runnable, however the current iteration will not be cancelled
> until the timeout value for
>   * the class has been reached. A cancellation will result in a test failure.
>   * @param runnable a Runnable that does not throw exceptions and
> responds to cancellation signal
>   * @param duration a maximum amount of time to repeat for
>   */
>  public void repeatForDuration(Runnable runnable, Integer duration);
> 
>  /**
>   * Set the timeout for the threads. After the timeout is reached,
> the threads will be interrupted
>   * and will throw a CancellationException
>   */
>  public void setTimeout(Integer seconds) {
>timeout = seconds;
>  }
> 
>  /**
>   * Runs all callables in toRun in parallel and fail if threads'
> conditions were not met. Runs
>   * until timeout is reached.
>   * @throws InterruptedException if interrupted before timeout
>   */
>  public void executeInParallel();
> 
>  /**
>   * Runs all callables in toRun in the order that they were added and
> fail if threads' conditions
>   * were not met. Runs until timeout is reached.
>   * @throws InterruptedException if interrupted before timeout
>   */
>  public void executeInSeries();
> }
> 
> // example usage:
> new ConcurrentTestHelper()
>   .runAndExpectException(runnable, UnsupportedOperationException.class)
>   .runAndExpectNoException(runnable)
>   .runAndExpectValue(callable, result)
>   .repeatUntilAllOtherRunnablesFinish(runnable)
>   .repeatFor(runnable, time)  //keep doing this for time amount of time
>   .repeat(runnable, iterations)  //keep doing this for N times
>   .setTimeout(seconds) //Throws exception if test takes too long
>   .executeInParallel();

—
Dale Emery
dem...@pivotal.io

Re: [DISCUSS] When is a test not flaky anymore?

[Dale Emery]

The pattern I’ve seen in lots of other organizations: When a few tests 
intermittently give different answers, people attribute the intermittence to 
the tests, quickly lose trust in the entire suite, and increasingly discount 
failures.

If we’re going to attend to every failure in the larger suite, then we won’t 
suffer that fate, and I’m in favor of deleting the Flaky tag.

Dale

> On Jul 5, 2018, at 8:15 PM, Dan Smith  wrote:
> 
> Honestly I've never liked the flaky category. What it means is that at some
> point in the past, we decided to put off tracking down and fixing a failure
> and now we're left with a bug number and a description and that's it.
> 
> I think we will be better off if we just get rid of the flaky category
> entirely. That way no one can label anything else as flaky and push it off
> for later, and if flaky tests fail again we will actually prioritize and
> fix them instead of ignoring them.
> 
> I think Patrick was looking at rerunning the flaky tests to see what is
> still failing. How about we just run the whole flaky suite some number of
> times (100?), fix whatever is still failing and close out and remove the
> category from the rest?
> 
> I think will we get more benefit from shaking out and fixing the issues we
> have in the current codebase than we will from carefully explaining the
> flaky failures from the past.
> 
> -Dan
> 
> On Thu, Jul 5, 2018 at 7:03 PM, Dale Emery  wrote:
> 
>> Hi Alexander and all,
>> 
>>> On Jul 5, 2018, at 5:11 PM, Alexander Murmann 
>> wrote:
>>> 
>>> Hi everyone!
>>> 
>>> Dan Smith started a discussion about shaking out more flaky DUnit tests.
>>> That's a great effort and I am happy it's happening.
>>> 
>>> As a corollary to that conversation I wonder what the criteria should be
>>> for a test to not be considered flaky any longer and have the category
>>> removed.
>>> 
>>> In general the bar should be fairly high. Even if a test only fails ~1 in
>>> 500 runs that's still a problem given how many tests we have.
>>> 
>>> I see two ends of the spectrum:
>>> 1. We have a good understanding why the test was flaky and think we fixed
>>> it.
>>> 2. We have a hard time reproducing the flaky behavior and have no good
>>> theory as to why the test might have shown flaky behavior.
>>> 
>>> In the first case I'd suggest to run the test ~100 times to get a little
>>> more confidence that we fixed the flaky behavior and then remove the
>>> category.
>> 
>> Here’s a test for case 1:
>> 
>> If we really understand why it was flaky, we will be able to:
>>- Identify the “faults”—the broken places in the code (whether system
>> code or test code).
>>- Identify the exact conditions under which those faults led to the
>> failures we observed.
>>- Explain how those faults, under those conditions. led to those
>> failures.
>>- Run unit tests that exercise the code under those same conditions,
>> and demonstrate that
>>  the formerly broken code now does the right thing.
>> 
>> If we’re lacking any of these things, I’d say we’re dealing with case 2.
>> 
>>> The second case is a lot more problematic. How often do we want to run a
>>> test like that before we decide that it might have been fixed since we
>> last
>>> saw it happen? Anything else we could/should do to verify the test
>> deserves
>>> our trust again?
>> 
>> 
>> I would want a clear, compelling explanation of the failures we observed.
>> 
>> Clear and compelling are subjective, of course. For me, clear and
>> compelling would include
>> descriptions of:
>>   - The faults in the code. What, specifically, was broken.
>>   - The specific conditions under which the code did the wrong thing.
>>   - How those faults, under those conditions, led to those failures.
>>   - How the fix either prevents those conditions, or causes the formerly
>> broken code to
>> now do the right thing.
>> 
>> Even if we don’t have all of these elements, we may have some of them.
>> That can help us
>> calibrate our confidence. But the elements work together. If we’re lacking
>> one, the others
>> are shaky, to some extent.
>> 
>> The more elements are missing in our explanation, the more times I’d want
>> to run the test
>> before trusting it.
>> 
>> Cheers,
>> Dale
>> 
>> —
>> Dale Emery
>> dem...@pivotal.io
>> 
>> 

—
Dale Emery
dem...@pivotal.io

Re: [DISCUSS] When is a test not flaky anymore?

[Dale Emery]

Hi Alexander and all,

> On Jul 5, 2018, at 5:11 PM, Alexander Murmann  wrote:
> 
> Hi everyone!
> 
> Dan Smith started a discussion about shaking out more flaky DUnit tests.
> That's a great effort and I am happy it's happening.
> 
> As a corollary to that conversation I wonder what the criteria should be
> for a test to not be considered flaky any longer and have the category
> removed.
> 
> In general the bar should be fairly high. Even if a test only fails ~1 in
> 500 runs that's still a problem given how many tests we have.
> 
> I see two ends of the spectrum:
> 1. We have a good understanding why the test was flaky and think we fixed
> it.
> 2. We have a hard time reproducing the flaky behavior and have no good
> theory as to why the test might have shown flaky behavior.
> 
> In the first case I'd suggest to run the test ~100 times to get a little
> more confidence that we fixed the flaky behavior and then remove the
> category.

Here’s a test for case 1:

If we really understand why it was flaky, we will be able to:
- Identify the “faults”—the broken places in the code (whether system code 
or test code).
- Identify the exact conditions under which those faults led to the 
failures we observed.
- Explain how those faults, under those conditions. led to those failures.
- Run unit tests that exercise the code under those same conditions, and 
demonstrate that
  the formerly broken code now does the right thing.

If we’re lacking any of these things, I’d say we’re dealing with case 2.

> The second case is a lot more problematic. How often do we want to run a
> test like that before we decide that it might have been fixed since we last
> saw it happen? Anything else we could/should do to verify the test deserves
> our trust again?


I would want a clear, compelling explanation of the failures we observed.

Clear and compelling are subjective, of course. For me, clear and compelling 
would include
descriptions of:
   - The faults in the code. What, specifically, was broken.
   - The specific conditions under which the code did the wrong thing.
   - How those faults, under those conditions, led to those failures.
   - How the fix either prevents those conditions, or causes the formerly 
broken code to
 now do the right thing.

Even if we don’t have all of these elements, we may have some of them. That can 
help us
calibrate our confidence. But the elements work together. If we’re lacking one, 
the others
are shaky, to some extent.

The more elements are missing in our explanation, the more times I’d want to 
run the test
before trusting it.

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io

Re: DISCUSS: Refactor test source set for integrationTest and distributedTest

[Dale Emery]

The two main ways to organize tests are hierarchies (such as packages or file 
folders) and tagging (such as tagging).

Each supports different purposes for selecting tests to run.

Hierarchies are good for stable, mutually exclusive purposes. Unit, 
Distributed, Integration, and Acceptance seem like pretty stable purposes for 
selecting tests. Another probably stable purpose is functional area, like when 
you want to run all of the tests for a particular area.

Those two stable purposes are orthogonal, which leads to somewhat replicated 
hierarchies: Each functional area has its own unit, distributed, integration, 
and acceptance tests, or vice versa. As long as the purposes really are stable, 
this replicated structure works just fine. If they’re not stable, it’s better 
(in the absence of Gradle doing unfortunate things when it finds and filters 
tests) to use tagging for the less-stable idea.

Tagging is good when you want to select tests *across* those more stable 
purposes, or *within* them. I don’t know enough about GemFire/Geode to know 
what things might be best represented by tags.

The key, I think, is to figure out which purposes are more stable, and use 
hierarchies to organize tests for those purposes, then use tagging for 
crosscutting or ad hoc selection.

—
Dale Emery
dem...@pivotal.io

> On Jun 26, 2018, at 4:34 PM, Patrick Rhomberg  wrote:
> 
> I like the idea of good structure around our test-complexity @Category
> layout.
> 
> @Alexander, not to speak for Bruce, but to my mind things like SecurityTest
> / WanTest / etc are orthogonal to the UnitTest / IntegrationTest /
> DistributedTest / AcceptanceTest classification.  I like adding better
> runtime and structure on the latter, but there's no real reason to
> sacrifice the former labeling.
> 
> @Anthony, I'm pretty confident we'll need a commonTest.  Off the top of my
> head, it'll need to host the startup rules, GfshRule, and the
> SimpleSecurityManager.
> 
> On Tue, Jun 26, 2018 at 4:22 PM, Anthony Baker  wrote:
> 
>> Sounds good, though I’m not entirely sure we need ‘commonTest/‘.  I guess
>> we’ll find out :-)
>> 
>> Anthony
>> 
>> 
>>> On Jun 26, 2018, at 4:19 PM, Dan Smith  wrote:
>>> 
>>> +1 for the suggested structure.
>>> 
>>> -Dan
>>> 
>>> On Tue, Jun 26, 2018 at 3:36 PM, Jacob Barrett 
>> wrote:
>>> 
>>>> I'd like to suggest that we refactor our current test source set, which
>>>> contains both unit, integration and distributed tests, into distinct
>> source
>>>> sets, test, integrationTest, distributedTest. These source sets would
>>>> replace the use of the primary categories UnitTest, IntegrationTest and
>>>> DistributedTest.
>>>> 
>>>> The catalyst for this change is an issue that Gradle's test runner
>> doesn't
>>>> pre-filter categories when launching tests, so if the tests are
>> launched in
>>>> separate JVMs or Docker containers, like :distributeTest task, the cost
>> of
>>>> spinning up those resources is realized only to immediately exit without
>>>> running any test for all test classes in the module. Switching to
>> separate
>>>> source sets for each category would remove the need to filter on
>> category
>>>> and only tests in the corresponding source set would get executed in
>> their
>>>> external JVM or Docker container. An example of this issue is
>>>> geode-junit:distributedTest task, which forks all test classes in
>> separate
>>>> JVMs but never actually runs any tests since there are no
>> DistributedTest
>>>> tagged tests.
>>>> 
>>>> The secondary effect is a way too isolate dependencies in each of the
>>>> source sets. Unit tests in the test set would not have dependencies need
>>>> for integration tests or distributed test so that if you accidentally
>> tried
>>>> to import classes from those frameworks you would get a compiler
>> failure.
>>>> Likewise, integration tests would not include distributed test framework
>>>> dependencies. Any shared test classes like mock, dummies, fakes, etc.
>> could
>>>> be shared in a commonTest source set, but would not contain any tests
>>>> itself.
>>>> 
>>>> The proposed structure would look like this:
>>>> 
>>>> test/ - only contains unit tests.
>>>> integrationTest/ - only contains integration style tests.
>>>> distributedTest/ - only includes DUnit based tests.
>>>> commonTest/ - includes commonly shared classes between each test
>> category.
>>>> Does not contain any classes.
>>>> 
>>>> Thoughts?
>>>> 
>>>> -Jake
>>>> 
>> 
>> 

PR: Configure Spotless not to join already-wrapped lines

[Dale Emery]

Hi all,

I've submitted a PR to configure Spotless to refrain from joining 
already-wrapped lines:
https://github.com/apache/geode/pull/1994 
<https://github.com/apache/geode/pull/1994>

My main motivation was to allow formatting method chains with one method per 
line (such as in long Stream pipelines, Mock configurations, and AssertJ 
assertions) when that would make the code easier to understand.

The change in my PR goes further than that. It causes Spotless not to join any 
lines if they are already shorter than the line length limit. I could not find 
an easy way to prevent joining lines only in method chains.

Comments?

Dale

—
Dale Emery
dem...@pivotal.io

Requesting Geode Wiki Edit Permissions

[Dale Emery]

Hello,

I am requesting permission to add and edit Geode Wiki pages.

My user ID on Apache Confluence is: demery

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io

Requesting Jira permissions

[Dale Emery]

Hi,

I am requesting Jira permission to assign Jira tickets to myself.

My user ID is demery

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io

Requesting access to Geode CI Pipeline

[Dale Emery]

Hello,

I am a new hire at Pivotal. I would like access to the Geode CI Pipeline. Can 
you add me?

My GitHub account is: dhemery

Cheers,
Dale

—
Dale Emery
dem...@pivotal.io